Anastasia added a comment. > This patch does not address the issue with the accessors > on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes), > because I don't know how to solve it without breaking a ton of > rather nice encapsulation. Either, every mention of compatiblyIncludes > must be replaced with a call to a corresponding ASTContext method, > Qualifiers must have a handle to ASTContext, or ASTContext must be > passed to every such call. This revision mentions the issue in a comment: > https://reviews.llvm.org/D49294
I think using ASTContext helper is ok then. I was just thinking about testing the new logic. Should we add something like `-ffake-address-space-map` (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces. ================ Comment at: lib/Sema/SemaOverload.cpp:3171 + // qualification conversion for disjoint address spaces make address space + // casts *work*? Qualifiers FromQuals = FromType.getQualifiers(); ---------------- I guess if address spaces don't overlap we don't have a valid qualification conversion. This seems to align with the logic for cv. My guess is that none of the other conversions will be valid for non overlapping address spaces and the error will occur. I think at this point we might not need to know if it's implicit or explicit? I believe we might have a separate check for this somewhere because it works for OpenCL. I don't know though if it might simplify the flow if we move this logic rather here. The cv checks above seem to use `CStyle` flag. I am wondering if we could use it to detect implicit or explicit. Because we can only cast address space with C style cast at the moment. Although after adding `addrspace_cast` operator that will no longer be the only way. ================ Comment at: lib/Sema/SemaOverload.cpp:5150 + // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is + // not qualified with an address space, but if there's no implicit conversion ---------------- Agree! We need to check that address spaces are not identical and then validity! But I guess it's ok for C++ because we can't add addr space qualifier to implicit object parameter yet? It's broken for OpenCL though! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62574/new/ https://reviews.llvm.org/D62574 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
