[PATCH] D62574: Add support for target-configurable address spaces.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.
Herald added a subscriber: bjope.

This change looks good to me in general conceptually as it is completing missing logic in clang that let's targets to customize the address space behavior!

The change also looks good from the implementation side apart from some small details raised in the comments.

The only thing is that it would be good to test the new target setting logic somehow... do you have any ideas in mind? We could think of creating a dummy target for that or adding a dummy setting in existing targets - maybe SPIR could be a candidate. We have done something similar in the past if you look at `FakeAddrSpaceMap` in `LangOpts`.



================
Comment at: clang/include/clang/AST/ASTContext.h:2371
+  /// can be safely used as an object qualified with A.
+  bool compatiblyIncludes(Qualifiers A, Qualifiers B) const {
+    return isAddressSpaceSupersetOf(A, B) &&
----------------
Perhaps not necessarily related to this change but I feel we should provide an explanation of how those functions behave wrt address spaces because terms more qualified or less qualified don't really apply there.


================
Comment at: clang/include/clang/AST/ASTContext.h:2588
 
+  /// Returns true if address space A is a superset of B.
+  /// The subspace/superspace relation governs the validity of implicit
----------------
I think we should finally start referring to section 5 of embedded C spec as it is not a secret that clang implements exactly this. It is rather a good thing to document the implemented behavior.


================
Comment at: clang/include/clang/AST/ASTContext.h:2612
+
+  /// Returns true if an explicit cast from address space A to B is legal.
+  /// Explicit conversion between address spaces is permitted if the address
----------------
Here we should say that this is an extension or enhancement of embedded C rules that clang implements.

Technically for OpenCL we could refactor to use this functionality as we don't support such explicit casts on disjoint address spaces. But then this would not be necessarily a target setting.


================
Comment at: clang/lib/AST/ASTContext.cpp:10959
+  // Otherwise, ask the target.
+  return Target->isAddressSpaceSupersetOf(A, B);
+}
----------------
I guess we should add a similar check here as below?

 
```
if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
      From == LangAS::Default || To == LangAS::Default)
```


================
Comment at: clang/lib/AST/ASTContext.cpp:10963
+bool
+ASTContext::isExplicitAddrSpaceConversionLegal(LangAS From, LangAS To) const {
+  // If From and To overlap, the cast is legal.
----------------
Btw I assume that explicit cast can't reject what is not rejected by implicit cast?

I am not sure if we need to enforce or document this somehow considering that we provide full configurability now?


================
Comment at: clang/lib/Sema/SemaCast.cpp:2423
   auto DestPointeeType = DestPtrType->getPointeeType();
-  if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
+  if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+        SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {
----------------
Btw just to point our that overlapping used to be a commutative operation so you could swap arguments and still get the same answer but for `isExplicitAddrSpaceConversionLegal` is not the same I assume?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:3235
   //  - in non-top levels it is not a valid conversion.
+  // FIXME: This should probably be using isExplicitAddrSpaceConversionLegal,
+  // but we don't know if this is an implicit or explicit conversion.
----------------
Sorry if this has been discussed previously, do you refer to the first or the second case and is there any failing test case?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:5289
 
+  // 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
----------------
Do you have a failing test case, if so feel free to create a bug?


================
Comment at: clang/test/CodeGenCXX/address-space-cast.cpp:41
+  // CHECK: %[[cast:.*]] = bitcast i32* %{{.*}} to i8*
+  // CHECK-NEXT: %[[cast2:.*]] = addrspacecast i8* %[[cast]] to i8 addrspace(5)*
+  // CHECK-NEXT: store i8 addrspace(5)* %[[cast2]]
----------------
I am confused about why this is changed now? adrspacecast can cast a type and an address space too i.e. it is implicitly a bitcast too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62574