Anastasia marked an inline comment as done. Anastasia added inline comments.
================ Comment at: lib/Sema/SemaCast.cpp:2309 + auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType( + DestPointeeType.getCanonicalType()); + return Self.Context.hasSameType(SrcPointeeTypeWithoutAS, ---------------- ebevhan wrote: > 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. > > Ok yes, I would like to formalize this indeed, but I will definitely need some help. I think the best way to proceed is to gather all the ideas we have discussed so far and send an RFC. 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
