ebevhan added inline comments.
================
Comment at: lib/Sema/SemaCast.cpp:2224
+ } else if (IsLValueCast) {
Kind = CK_LValueBitCast;
} else if (DestType->isObjCObjectPointerType()) {
----------------
Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > Anastasia wrote:
> > > > ebevhan wrote:
> > > > > Anastasia wrote:
> > > > > > ebevhan wrote:
> > > > > > > This might not be applicable to this patch, but just something I
> > > > > > > noticed.
> > > > > > >
> > > > > > > So `reinterpret_cast` only operates on pointers when dealing with
> > > > > > > address spaces. What about something like
> > > > > > > ```
> > > > > > > T a;
> > > > > > > T& a_ref = reinterpret_cast<T&>(a);
> > > > > > > ```
> > > > > > > `reinterpret_cast` on an lvalue like this is equivalent to
> > > > > > > `*reinterpret_cast<T*>(&a)`. So for AS code:
> > > > > > > ```
> > > > > > > __private T x;
> > > > > > > __generic T& ref = reinterpret_cast<__generic T&>(x);
> > > > > > > ```
> > > > > > > This should work, since `*reinterpret_cast<__generic T*>(&x)` is
> > > > > > > valid, correct?
> > > > > > >
> > > > > > > What if we have the reference cast case with a different address
> > > > > > > space like this? Doesn't the `IsLValueCast` check need to be
> > > > > > > first?
> > > > > > >
> > > > > > > What if we have the reference cast case with a different address
> > > > > > > space like this? Doesn't the IsLValueCast check need to be first?
> > > > > >
> > > > > > Ok, let me see if I understand you. I changed `__generic` ->
> > > > > > `__global` since it's invalid and the error is produced as follows:
> > > > > > test.cl:7:21: error: reinterpret_cast from 'int' to '__global int
> > > > > > &' is not allowed
> > > > > > __global int& ref = reinterpret_cast<__global int&>(x);
> > > > > >
> > > > > > Is this not what we are expecting here?
> > > > > My last sentence could have been a bit clearer. Yes, for the
> > > > > `__private`->`__global` case, it should error. But I was thinking
> > > > > more of the case where the AS conversion is valid, like
> > > > > `__private`->`__generic`. Then we will do the AS conversion, but we
> > > > > should have done both an AS conversion and an `LValueBitCast`,
> > > > > because we need to both convert the 'pointer' and also dereference it.
> > > > Hmm... it seems like here we can only have one cast kind... I guess
> > > > `CK_LValueBitCast` leads to the generation of `bitcast` in the IR? But
> > > > `addrspacecast` can perform both bit reinterpretation and address
> > > > translation, so perhaps it makes sense to only have an address space
> > > > conversion in this case? Unless some other logic is needed elsewhere
> > > > before CodeGen... I will try to construct a test case in plain C++.
> > > I am not sure I understood the problem you are describing, may be I need
> > > an example...
> > >
> > > But the valid address space conversion example in OpenCL
> > >
> > > ```
> > > __private T x = ...;
> > > __generic T& ref = reinterpret_cast<__generic T&>(x);
> > > ```
> > > produces correctly `addrspacecast`
> > >
> > > ```
> > > %0 = load i32*, i32** %x.addr, align 4
> > > %1 = addrspacecast i32* %0 to i32 addrspace(4)*
> > > store i32 addrspace(4)* %1, i32 addrspace(4)** %ref
> > > ```
> > >
> > > However, if we keep checking `CK_LValueBitCast` first clang attempts to
> > > generate `bitcast` but fails because pointers are in different address
> > > spaces.
> > > Hmm... it seems like here we can only have one cast kind... I guess
> > > CK_LValueBitCast leads to the generation of bitcast in the IR?
> >
> > That's likely, yes. The dereference in the example disappears if we assign
> > to a `T&` and it becomes implied if we assign to a `T` through
> > lvalue-to-rvalue conversion, so in both cases we're taking the address of
> > something and then converting the address to a different type.
> >
> > > But addrspacecast can perform both bit reinterpretation and address
> > > translation, so perhaps it makes sense to only have an address space
> > > conversion in this case? Unless some other logic is needed elsewhere
> > > before CodeGen... I will try to construct a test case in plain C++.
> >
> > Oh, you're right! For some reason I was thinking that `addrspacecast` could
> > only change the addrspace of the argument, but apparently it could change
> > the pointee type too. At least according to the langref. So long as nothing
> > in Clang assumes that a CK_AddressSpaceConversion can't change the pointee
> > type as well as the address space I guess it should be safe.
> >
> > When I try a 'simultaneous' conversion in C, I don't get a single
> > addrspacecast, though: https://godbolt.org/z/Q818yW So I wonder if existing
> > code handles this.
> >
> > A test case would be good.
> Sorry, I haven't seen your previous comment before sending mine. :)
>
>
> >
> > When I try a 'simultaneous' conversion in C, I don't get a single
> > addrspacecast
>
> I think 2 conversions in IR are not necessary, not sure how we ended up doing
> this though... but we had related discussion on that while implementing some
> OpenCL features and the conclusion was that we should just have an
> `addrspacecast`. Do you think we should correct this in C?
>
>
>
> > When I try a 'simultaneous' conversion in C, I don't get a single
> > addrspacecast, though: https://godbolt.org/z/Q818yW So I wonder if existing
> > code handles this.
>
> Ok, I will check whether we cover this with any CodeGen test and if not I
> will add one.
>
>
> I think 2 conversions in IR are not necessary, not sure how we ended up doing
> this though... but we had related discussion on that while implementing some
> OpenCL features and the conclusion was that we should just have an
> addrspacecast. Do you think we should correct this in C?
Yes, I don't think it's necessary either, the addrspacecast should be enough.
Looking closer at the emission in the C case, Clang actually emits a single
addrspacecast. Something else in LLVM is splitting it up into an addrspacecast
and a bitcast. So it seems to be working just fine.
I was just concerned that we might hit edge cases where one conversion wasn't
enough to convert both the address space and the pointee type, but it seems
like CK_AddressSpaceConversion acts as both if it needs to. So it all seems
good!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58346/new/
https://reviews.llvm.org/D58346
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
