ebevhan added inline comments.
================ Comment at: lib/Sema/SemaCast.cpp:2309 + auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType( + DestPointeeType.getCanonicalType()); + return Self.Context.hasSameType(SrcPointeeTypeWithoutAS, ---------------- Anastasia wrote: > rjmccall wrote: > > ebevhan wrote: > > > 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! > > It seems to me that the rules in this function are the reasonable > > cross-language rules. If you want to check for OpenCL at the top as a > > fast-path check (taking advantage of that fact that no other language modes > > currently have overlapping address spaces), that might be reasonable, but > > the comment and indentation should reflect that. > Btw, I attempted to remove the OpenCL check and unfortunately found failure > in the following test: **test/CodeGenCXX/address-space-cast.cpp** > > > ``` > error: C-style cast from 'char *' to '__attribute__((address_space(5))) char > *' converts between mismatching address spaces > __private__ char *priv_char_ptr = (__private__ char *)gen_char_ptr; > ``` > > I am not sure what such conversion does but I feel if I want to update it I > might need to get wider agreement. I think sending an RFC might be a good way > forward. > Seems like that would happen if the address spaces don't overlap, which none of the target address spaces do. That sort of cast works in C because the ASes are only tested for compatibility when you're building for OpenCL (in SemaCast:checkAddressSpaceCast). Without a guard on OpenCL, any target AS compatibility check will fail. A possible fix could be to omit the `isAddressSpaceOverlapping` check when `!OpenCL`, but properly formalizing the AS conversion behavior would most likely be the best fix for this. 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