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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
