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

Mon Oct 12 06:34:23 PDT 2020

Anastasia added a comment.

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

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

Ok yes if that can be done it would be better. However, it might need extra involvement from AMD side to make sure at least they would agree on maintaining this.

================
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
----------------
ebevhan wrote:
> 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.
Technically Clang's policy is to implement documented and standardized behavior. So the question is not necessarily about its usefulness, but about adhering to good practices. 

I think it is ok to deviate from the standard to make the behavior more helpful in some cases but we should aim at documenting such changes. This will help the developers in the future if they need to fix bugs or build something on top.

================
Comment at: clang/lib/AST/ASTContext.cpp:10959
+  // Otherwise, ask the target.
+  return Target->isAddressSpaceSupersetOf(A, B);
+}
----------------
ebevhan wrote:
> 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.
I am not sure I follow your comment.

> Is it not useful for targets to be able to express relations of LangASes and target ASes?

yes that's why I was thinking we should add a check to make sure we only ask target for target ASes...

However, I am not sure what we should do if there is a mix of target and language AS. I guess this should be governed by the target hooks?

================
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.
----------------
ebevhan wrote:
> 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.
Ok, we could update to:
`
 This should probably be using isExplicitAddrSpaceConversionLegal -> The first conversion should probably be using isExplicitAddrSpaceConversionLegal too.`

If you already have a failing test case we could create a bug to track this better.

================
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
----------------
ebevhan wrote:
> 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.
Ok, that's right in OpenCL almost every type gets an address space early in parsing.

But if we already know it's a bug and the fix for it we could change this code? Although I think the bug fix should better be done on a separate review.

================
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]]
----------------
ebevhan wrote:
> 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.
Ah ok, makes sense although the IR will be different to C, but the sematic is the same.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62574