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:
> > > 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++.
> 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.
================
Comment at: lib/Sema/SemaCast.cpp:2309
+ auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType(
+ DestPointeeType.getCanonicalType());
+ return Self.Context.hasSameType(SrcPointeeTypeWithoutAS,
----------------
Anastasia wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > Maybe I'm mistaken, but won't getting the canonical type here drop
> > > qualifiers (like cv) in nested pointers? If so, an addrspace_cast might
> > > strip qualifiers by mistake.
> > Yes, indeed I will need to extend this to nested pointers when we are
> > ready. But for now I can try to change this bit... however I am not sure it
> > will work w/o canonical types when we have typedef. I will try to create an
> > example and see.
> I checked the canonical type does preserve the qualifiers correctly.
>
> Here is the AST dump of the following C type `mytype const __generic*` where
> `typedef __generic int* mytype;`.
>
>
> ```
> PointerType 0x204d3b0 'const __generic mytype *'
> `-QualType 0x204d369 'const __generic mytype' const __generic
> `-TypedefType 0x204d320 'mytype' sugar
> |-Typedef 0x204d1b0 'mytype'
> `-PointerType 0x204d170 '__generic int *'
> `-QualType 0x204d158 '__generic int' __generic
> `-BuiltinType 0x2009750 'int'
> ```
>
> and it's canonical representation in AST is:
>
> ```
> PointerType 0x204d380 '__generic int *const __generic *'
> `-QualType 0x204d349 '__generic int *const __generic' const __generic
> `-PointerType 0x204d170 '__generic int *'
> `-QualType 0x204d158 '__generic int' __generic
> `-BuiltinType 0x2009750 'int'
> ```
>
> So using canonical type will just simply handling of nested pointer chain by
> avoiding special casing typedefs. We won't loose any qualifiers.
Okay, seems good then!
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
