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

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:486
+                                       bool IsSYCLOrOpenCL = false) {
+    if (ASMap) {
+      bool IsATargetAS = false;
----------------
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.


================
Comment at: clang/include/clang/AST/Type.h:489
+      bool IsBTargetAS = false;
+      if (A > LangAS::FirstTargetAddressSpace)
+        IsATargetAS = true;
----------------
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).


================
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)
----------------
`getLangASFromTargetAS((*ASMap)[static_cast<unsigned>(LangAS::opencl_constant)])` 


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


================
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
----------------
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));
```


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


================
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()) &&
----------------
Do we need to pass `ASMap` and `IsSYCLOrOpenCL` here, too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124382