[PATCH] D111734: [HIP] Relax conditions for address space cast in builtin args

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.

LGTM in general, modulo a readability suggestion.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:6548-6550
+        // Prevent addrspace cast if the parameter has a default address
+        // space, or the argument has a non-default addrspace and the
+        // target addrspaces of the argument and the parameter differ.
----------------
tra wrote:
> I'd rephrase it in terms of `add ASC if target AS is different`.
> 
> `Prevent` assumes that we've already decided to add an ASC, while it is not the case. All we know is that we've got a pointer argument and we are figuring out whether ASC is needed.
Hmm. I guess that does not quite match what we do here.
Oringal code would add ASC for generic->specific only.
The test cases you've addedd are for specific->specific AS, AFAICT, and that makes me wonder if I understand what's going on here.

So the test case currently does fail, because we end up w/o implicit ASC and end up trying to pass wrong types of pointers. https://godbolt.org/z/MaWMbGb5z

Now we actually want to allow implicit ASC for the pointer types that are in the *same* target AS, so my comment suggestion was actually wrong.

This confusion also suggests that the condition is hard to understand. Perhaps we can rewrite it.

E.g.
```
  bool NeedImplicitASC = 
        ParamAS != LangAS::Default &&   // Pointers params in generic AS don't need special handling.
        ( ArgAs == LangAS::Default  ||         // We do allow implicit conversion from generic AS 
          // or from specific AS which has target AS matching that of Param.
          getASTContext().getTargetAddressSpace(ArgAS) == getASTContext().getTargetAddressSpace(ParamAS));
  if (!NeedImplicitASC)
     continue;
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111734