[SYCL] Fix builtins address space type #4275
Conversation
This patch fixes the `core/GroupAsyncCopy.cl` and `ocl/prefetch.cl` lit tests in `libclc`. These started failing with the introduction of the SYCL address spaces in intel#3634. The issue is that the builtins are declared with the SYCL address spaces in the tablegen so when used in an OpenCL setting it ends up with address space mismatches between things like `opencl_global` and `sycl_global`. These two were the only tests affected because they're also the only tested builtins that do not have generic address space variants in tablegen, the other tests would just fall back on the generic address space variants. With this patch the builtins should automatically get the appropriate version of the address space depending on the context.
npmiller
requested review from
AaronBallman,
elizabethandrews and
premanandrao
as code owners
|
@premanandrao @AaronBallman @bader please take a look. I am not too familiar with address space handling. |
It looks good to me. But I would like @bader to confirm. |
Well this is exercised in Also this is not really a problem in SYCL, rather it's a problem when using SPIR-V builtins with |
Understood, but this is still a functional change to SYCL, so it should come with test coverage. Otherwise, we increase the maintenance burden for folks working in this repo because they can't run local tests when making changes, so we run the risk of accidentally regressing behavior (and hopefully catch it when running check-libclc as part of check-sycl from CI some day). Note that check-sycl is not where I'd want to see the test coverage anyway; the changes are to Clang, so I'm hoping to see check-clang coverage if possible. |
Oh I see, that makes sense, I'll add some specific clang tests for this. |
|
I've had to update the lit tests patterns for windows, and it also turns out that the microsoft mangler didn't know how to mangle SYCL address spaces, so I'm not sure how that worked before. So I've also added a patch to the microsoft mangler to add the SYCL address spaces and hopefully that works, I don't have a windows build on hand to test that further but maybe we can see what the CI says before I set that up. |
This patch fixes the
core/GroupAsyncCopy.clandocl/prefetch.cllittests in
libclc.These started failing with the introduction of the SYCL address spaces
in #3634.
The issue is that the builtins are declared with the SYCL address spaces
in the tablegen so when used in an OpenCL setting it ends up with
address space mismatches between things like
opencl_globalandsycl_global.These two were the only tests affected because they're also the only
tested builtins that do not have generic address space variants in
tablegen, the other tests would just fall back on the generic address
space variants.
With this patch the builtins should automatically get the appropriate
version of the address space depending on the context.