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

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D62574#2242471 <https://reviews.llvm.org/D62574#2242471>, @Anastasia wrote:

> 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`.

Perhaps we could add a configuration to AMDGPU? That has address spaces.

I'm not a big fan of adding an option just for testing.



================
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
----------------
Anastasia wrote:
> 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.
I'm still a bit on the fence about what Embedded-C really stipulates. I don't think it's against the spec to simply disallow disjoint conversion altogether, but it's only necessary to keep Clang's current implementation working.


================
Comment at: clang/lib/AST/ASTContext.cpp:10959
+  // Otherwise, ask the target.
+  return Target->isAddressSpaceSupersetOf(A, B);
+}
----------------
Anastasia wrote:
> I guess we should add a similar check here as below?
> 
>  
> ```
> if (isTargetAddressSpace(From) || isTargetAddressSpace(To) ||
>       From == LangAS::Default || To == LangAS::Default)
> ```
Is it not useful for targets to be able to express relations of LangASes and target ASes?

The method below must be guarded because otherwise all casts between LangASes would be legal.


================
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.
----------------
Anastasia wrote:
> 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?
It shouldn't do that, no. I don't think there's any way to guarantee this, though.

I could add something to the target methods about it.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2423
   auto DestPointeeType = DestPtrType->getPointeeType();
-  if (!DestPointeeType.isAddressSpaceOverlapping(SrcPointeeType)) {
+  if (!Self.Context.isExplicitAddrSpaceConversionLegal(
+        SrcPointeeType.getQualifiers(), DestPointeeType.getQualifiers())) {
----------------
Anastasia wrote:
> 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?
Correct, isExplicitAddrSpaceConversionLegal doesn't have to be commutative.


================
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.
----------------
Anastasia wrote:
> Sorry if this has been discussed previously, do you refer to the first or the second case and is there any failing test case?
It refers to the first case of "valid to convert to addr space that is a superset in all cases". Technically, it could be permitted even if the addr space is not a superset, if this is an explicit cast. But we don't know that. We only know if it's a c-style cast, because those are always 'explicit'.

I don't have a test case, unfortunately. I just made this observation as I was redoing all of the overlap/superspace checks. It might not even be a problem.


================
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
----------------
Anastasia wrote:
> Do you have a failing test case, if so feel free to create a bug?
Unsure how I'd make one. I suspect this can't be triggered in OpenCL++, because you can't really have LangAS::Default on FromType there, can you? It would always be some AS.

Doing it in another way would require a target that has configurable ASes, which doesn't exist yet. Also, it would require methods qualified with target ASes, and that doesn't work yet either.


================
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]]
----------------
Anastasia wrote:
> 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.
I don't remember the exact details of how it ends up this way, but it has to do with the way standard conversion sequences are determined. The bitcast and AS-cast are done in two separate steps; the bitcast as part of the pointer conversion in the second SCS step, and the AS-cast as part of the qualification conversion in the third SCS step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62574