__spirv_ macro added in 9ce77255b9a48 conflict with namespace in sycl headers

[jsji]

Describe the bug

We have __spirv namespace defined in sycl/include/sycl/__spirv/spirv_vars.hpp

9adce8ad68b7 revert 9ce7725 temporarily for now,
we should seek a solution for this.

To reproduce

git cherry-pick 9ce7725
build compiler
cd build
bin/clang++ -Werror -fsycl -fsycl-targets=spirv64 --offload-new-driver ../sycl/test-e2e/NewOffloadDriver/buffer.cpp

[jsji]

@intel/dpcpp-cfe-reviewers FYI.

[AlexeySachkov]

we should seek a solution for this.

Switching to SPIR-V backend which uses a separate target should do the trick in the long term.

Overall, we can maybe argue that the patch is disruptive for the OpenCL ecosystem: SPIRV-LLVM-Translator (which is used by several vendors) has what's called SPIR-V friendly LLVM IR where it looks for calls to functions prefixed with __spirv_ to translate them into the corresponding SPIR-V instructions.

The commit effectively completely breaks this workflow.

[jsji]

@intel/dpcpp-cfe-reviewers Note that we should try to resolve this conflict as early as possible, since more and more HLSL commits are depending on this define now. eg: 5a45711

FYI. @AaronBallman @bader

[AaronBallman]

Do I understand correctly that __spirv__ is only needed because of HLSL compatibility? If so, then it should only be defined in an HLSL target; we already have __SPIRV__ for detecting SPIR-V use, right? So the second need is just a confusing macro.

[gmlueck]

@gmlueck: Tagging myself to get notifications on this issue.

[jsji]

Do I understand correctly that __spirv__ is only needed because of HLSL compatibility? If so, then it should only be defined in an HLSL target; we already have __SPIRV__ for detecting SPIR-V use, right? So the second need is just a confusing macro.

From the commit
9ce7725

looks like it was the only reason.

[AaronBallman]

I left a comment on llvm/llvm-project#132848 (comment) to see if there's something to be done about this upstream.

[jsji]

I left a comment on llvm/llvm-project#132848 (comment) to see if there's something to be done about this upstream.

Thanks @AaronBallman

[bader]

@gmlueck: Tagging myself to get notifications on this issue.

FYI.

[jsji]

Should be fixed in llvm/llvm-project#142401

[mikolaj-pirog]

The 9ce7725 has been relanded in 5136554