[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

arsenm requested changes to this revision. arsenm added a comment. This revision now requires changes to proceed. Conclusion seems to be this should have a separate cast operation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151087/new/

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

rjmccall added a comment. Right, that's been my thinking, too. The CVR qualifiers are all "view" qualifiers: both the underlying object and the reference to it are assumed to be the same independent of qualifiers, and the qualifiers just affect the rules around accesses. C++'s rules around

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

aaron.ballman added a comment. In D151087#4363503 , @ebevhan wrote: > In D151087#4362059 , @aaron.ballman > wrote: > >> Based on all this, I think we should go with `__addrspace_cast` as a named >> cast and not

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

arsenm added a comment. In D151087#4363503 , @ebevhan wrote: > In D151087#4362059 , @aaron.ballman > wrote: > >> Based on all this, I think we should go with `__addrspace_cast` as a named >> cast and not allow

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

ebevhan added a comment. In D151087#4362059 , @aaron.ballman wrote: > Based on all this, I think we should go with `__addrspace_cast` as a named > cast and not allow the conversion through `reinterpret_cast` unless going > to/from a `[u]intptr_t`. I

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. We should at least be able to `reinterpret_cast` between cases we know are compatible, as the OpenCL check does. One of the problems with the numerical address space is it doesn't have any information to know if that's strictly legal or not. I'm not sure if casting

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

arsenm added a comment. In D151087#4361237 , @ebevhan wrote: > What is now a reinterpret_cast? An address space conversion, or a bitcast? > It's not as straightforward as it might seem. This is the most straightforward part. It's a bitcast.

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

aaron.ballman added a comment. The address space feature is governed by TR 18037 (https://standards.iso.org/ittf/PubliclyAvailableStandards/c051126_ISO_IEC_TR_18037_2008.zip), see section 5. The TR says (in part): > A non-null pointer into an address space A can be cast to a pointer into >

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

rjmccall added a comment. It's weird to have C-style casts that can't be done with any combination of named casts, so I think this makes some sense. I do think it should be limited to numbered address spaces of the same size, though, rather than basing it on whether we're in OpenCL mode.

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

Anastasia added a comment. I don't feel `reinterpret_cast` is the right fit for this as it's expected to do just reinterpretation but `addrspacecast` LLVM instruction which Clang generates might result in some more operations in particular some special instructions for address calculation.

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4361367 , @ebevhan wrote: > I don't think the standard argument really holds. It doesn't mention > restrictions on address spaces because it doesn't have to, since they don't > exist in C++. If they did, I'm pretty

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

ebevhan added a comment. In D151087#4361263 , @jhuber6 wrote: > I'd rather have an operation whose semantics are a little dangerous than > something that doesn't work at all. As it stands we need to use C-style casts > for this and I don't think

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4361237 , @ebevhan wrote: > Clang hasn't needed to formalize all of the address space behavior because > it's managed to piggyback off of the language semantics provided by OpenCL, > and no targets really have had a

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

ebevhan added a comment. In D151087#4360859 , @jhuber6 wrote: > You can have address spaced in freestanding C++, they just need to be > assigned according to the backens, e.g. https://godbolt.org/z/ahazae6Ta. And > I don't think that's a desirable

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

JonChesterfield added a comment. I like the current behaviour of bypass semantic checking and emit IR with the same number on it as my primary use case for feeding freestanding C++ to clang is as a convenient way to emit specific IR and I'm not that bothered about clang detecting mistakes

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4361081 , @arsenm wrote: > They bypass all semantic checks. For example if you declare something as > address space 4, it will let you write to it unlike __constant__. It will let > you place stack objects in globals

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

arsenm added a comment. In D151087#4360932 , @jhuber6 wrote: > How are they broken? The expectation is just that they line up with what the > backend defines them as, which should be a stable target. We could > potentially use target info to map the

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4360919 , @arsenm wrote: > In D151087#4360695 , @jhuber6 wrote: > >> I don't think that's something we can diagnose here with just the address >> space number. it would

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

arsenm added a comment. In D151087#4360695 , @jhuber6 wrote: > I don't think that's something we can diagnose here with just the address > space number. it would require information from the underlying target for the > expected pointer qualities to the

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4360818 , @ebevhan wrote: > By "freestanding C++" I assume you mean "C++ without the OpenCL C++ > extension" and not "C++ without extensions at all", because in the latter > case, you don't have address spaces

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

ebevhan added a comment. In D151087#4360743 , @jhuber6 wrote: > The problem is we don't have `addrspace_cast` in freestanding C++, so as it > stands we currently have no way to perform this operation in C++ which is > preventing me from implementing

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4360729 , @ebevhan wrote: > That's fair. I would like clang to improve and formalize the semantics for > generic address space behavior a bit, which was part of the point with D62574 >

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

ebevhan added a comment. In D151087#4360606 , @jhuber6 wrote: > In D151087#4360577 , @ebevhan wrote: > >> What would be the semantics of such an operation if the address spaces are >> disjoint? Or, if the

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4360668 , @arsenm wrote: >> It would most likely invalid, but I'm not asserting that `clang` should be >> responsible for diagnosing misuse in these cases. Especially because in >> generic freestanding C++ we don't

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

arsenm added a comment. In D151087#4360606 , @jhuber6 wrote: > In D151087#4360577 , @ebevhan wrote: > >> What would be the semantics of such an operation if the address spaces are >> disjoint? Or, if the

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 added a comment. In D151087#4360577 , @ebevhan wrote: > What would be the semantics of such an operation if the address spaces are > disjoint? Or, if the underlying pointer widths aren't the same? It would most likely invalid, but I'm not

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

ebevhan added a comment. What would be the semantics of such an operation if the address spaces are disjoint? Or, if the underlying pointer widths aren't the same? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151087/new/

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

jhuber6 created this revision. jhuber6 added reviewers: rjmccall, ebevhan, jdoerfert, JonChesterfield, aaron.ballman. Herald added subscribers: arichardson, Anastasia. Herald added a project: All. jhuber6 requested review of this revision. Herald added a project: clang. Herald added a subscriber: