[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;
----------------
jchlanda wrote:
> 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?
> 
> 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've changed that bool flag to be an enum specifying OpenCL/SYCL/None. The rational here is that the handling of AS values differs slightly (SYCL introduces `globa device` and `global host`). It would appear that CUDA follows completely different code-path and by the time `isAddressSpaceSuperset` called all of the language AS values are stripped and set to `0`, which is why (along the fact that we don't have a valid use case for CUDA) I left it out, and only return true for an exact match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124382