[PATCH] D124382: [Clang] Recognize target address space in superset calculation

[Jakub Chlanda via Phabricator via cfe-commits]

jchlanda added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:486
+                                       bool IsSYCLOrOpenCL = false) {
+    if (ASMap) {
+      bool IsATargetAS = false;
----------------
tra wrote:
> If A and B are both target AS, we fall through to the code which is dealing with language AS, which would not do us any good. If that's not expected to happen, we should have an assert to ensure it.
> 
> Next, I'm not particularly fond of `IsSYCLOrOpenCL`. Do we need it at all. If we do know that AS maps to OpenCL `Constant` or `Generic`, I would assume that those AS would follow the same semantics. Besides, will we ever see OpenCL language AS in non-OpenCL code?
> 
> Next, the function *is* OpenCL specific and would not work for CUDA or HIP. I think it needs to be generalized to provide language-specific AS mapping rules.
I would only like to handle the mixed AS case, it feels like trying to walk back from both HW AS and potentially do the logic of global and constant would be against the intention of users. Asserting on only one HW AS could backfire, as I think it should be allowed to assign between different HW AS.

The reason I added `IsSYCLOrOpenCL` is because this code is also exercised by `checkPointerTypesForAssignment` which is not OpenCL specific, so I had to have a way of conditionally enabling the conversion to generic AS.

I agree, it is too restrictive now, especially that the AS map provides values for SYCL, OpenCL and CUDA, so perhaps I should extend `IsSYCLOrOpenCL` to be an enum specifying which language the function deals with and act accordingly?



================
Comment at: clang/include/clang/AST/Type.h:489
+      bool IsBTargetAS = false;
+      if (A > LangAS::FirstTargetAddressSpace)
+        IsATargetAS = true;
----------------
tra wrote:
> Is the check intended to tell if A is a target AS? If so, we do have `isTargetAddressSpace()` for that (and it uses '>= LangAS::FirstTargetAddressSpace', which suggests that `>` may be incorrect, too).
Yeap, will update to `isTargetAddressSpace`.



================
Comment at: clang/include/clang/AST/Type.h:498-499
+        LangAS Constant = static_cast<LangAS>(
+            (*ASMap)[static_cast<unsigned>(LangAS::opencl_constant)] +
+            static_cast<unsigned>(LangAS::FirstTargetAddressSpace));
+        if (IsATargetAS)
----------------
tra wrote:
> `getLangASFromTargetAS((*ASMap)[static_cast<unsigned>(LangAS::opencl_constant)])` 
OK.


================
Comment at: clang/include/clang/AST/Type.h:500
+            static_cast<unsigned>(LangAS::FirstTargetAddressSpace));
+        if (IsATargetAS)
+          B = static_cast<LangAS>(
----------------
tra wrote:
> `if (!IsBTargetAS)` would be more directly related to what we're doing here.
You are right, will update.


================
Comment at: clang/include/clang/AST/Type.h:508
+              static_cast<unsigned>(LangAS::FirstTargetAddressSpace));
+        // When dealing with target AS return true if:
+        // * A is equal to B, or
----------------
tra wrote:
> Is the code above intended to ensure that both A and B are target AS at this point?
> If so, then it could be simplified to something like this:
> ```
> if (ASMap) {
>   if (!isTargetAddressSpace(A))
>     A = getLangASFromTargetAS((*ASMap)[static_cast<unsigned>(A)]);
>   if (!isTargetAddressSpace(B))
>     B = getLangASFromTargetAS((*ASMap)[static_cast<unsigned>(B)]);
> 
>   Generic = getLangASFromTargetAS((*ASMap)[static_cast<unsigned>(LangAS::opencl_generic)])
>   Constant = getLangASFromTargetAS((*ASMap)[static_cast<unsigned>(LangAS::opencl_constant)]);
> 
>   // ... proceed inferring whether A is superset of B in target AS.
>   return;
> }
> assert (isTargetAddressSpace(A) && isTargetAddressSpace(B));
> ```
Yes at the end of AS map accesses all address spaces have to be expressed in therms of HW values, but I want it to happen only in the case of mixed AS (Language and HW). I will add assert and use helpers, like you suggested in the snippet, but would like to keep the `^` condition.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9204
+  const LangASMap &ASMap = S.Context.getTargetInfo().getAddressSpaceMap();
+  if (!lhq.compatiblyIncludes(rhq, &ASMap)) {
+    const bool AddressSpaceSuperset = Qualifiers::isAddressSpaceSupersetOf(
----------------
tra wrote:
> Should you pass `IsSYCLOrOpenCL` to it too? The way `isAddressSpaceSupersetOf` is implemented now it may give you a different result without it. 
> 
> Also, it may make sense to plumb ASMap into `lhq.isAddressSpaceSupersetOf`, too, and just use the old code + couple of new arguments.
Yes, will add.

I thought it would introduce much bigger diff, and there was already a handy `static` version of it, don't mind modifying the member if you'd prefer.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9219
     // and from void*.
-    else if (lhq.withoutObjCGCAttr().withoutObjCLifetime()
-                        .compatiblyIncludes(
-                                rhq.withoutObjCGCAttr().withoutObjCLifetime())
-             && (lhptee->isVoidType() || rhptee->isVoidType()))
+    else if (lhq.withoutObjCGCAttr().withoutObjCLifetime().compatiblyIncludes(
+                 rhq.withoutObjCGCAttr().withoutObjCLifetime()) &&
----------------
tra wrote:
> Do we need to pass `ASMap` and `IsSYCLOrOpenCL` here, too?
I feel like it would be covered by the first `if` in this block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124382