[PATCH] D62574: Initial draft of target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D62574#1534159 <https://reviews.llvm.org/D62574#1534159>, @ebevhan wrote:

> >> I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.
> >> 
> >>> 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.
> >> 
> >> Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?
> > 
> > Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test?
>
> Well, since there is a mapping from the OpenCL address spaces to the AMDGPU target spaces, I would presume that if the target spaces were used on their own (via `__attribute__((address_space(1)))` for example) they should behave similarly to their OpenCL counterparts. It wouldn't have to be much, just a couple of type definitions and checks for implicit/explicit cast behavior.
>
> There's also the x86 case I mentioned. I'm not really sure how that feature is used, though.


Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces.



================
Comment at: include/clang/AST/ASTContext.h:2598
+  /// Returns true if address space A overlaps with B.
+  bool isAddressSpaceOverlapping(LangAS A, LangAS B) const {
+    // A overlaps with B if either is a superset of the other.
----------------
Is there any advantage of keeping superset&subset concept? Amd if yes, how do we position the new functionality with explicit cast?

I think I am missing a bit conceptual view... because I think we originally discussed to switch to implicit/explicit conversion model. Perhaps there is no reason to do it but I would just like to understand why? 


================
Comment at: lib/Sema/SemaCast.cpp:2224
+    // the cast is explicitly legal as well.
+    if (CStyle ? !Self.Context.isExplicitAddrSpaceConversionLegal(SrcQ, DestQ)
+               : !Self.Context.isAddressSpaceSupersetOf(DestQ, SrcQ)) {
----------------
It seems like you are changing the functionality here. Don't we need any test for this?


================
Comment at: lib/Sema/SemaCast.cpp:2319
+
+  Kind = CK_AddressSpaceConversion;
+  return TC_Success;
----------------
Btw I think this is set in:
https://reviews.llvm.org/D62299

Although I don't have any objections to changing to this.


================
Comment at: lib/Sema/SemaCast.cpp:2334
   // warn even though local -> generic is permitted.
-  if (Self.getLangOpts().OpenCL) {
-    const Type *DestPtr, *SrcPtr;
----------------
I think you might need to update the comment above to reflect that you are generalizing the behavior.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62574/new/

https://reviews.llvm.org/D62574