[SPIRV] Change how to detect OpenCL/Vulkan Env and update tests accordingly. #129689

maarquitos14 · 2025-03-04T11:38:53Z

A new test added for spirv-friendly builtins for SPV_KHR_bit_instructions unveiled that current mechanism to detect whether SPIRV Backend is in OpenCL environment or Vulkan environment was not good enough. This PR updates how to detect the environment and all the tests accordingly.

UPDATE: the new approach is having a new member in SPIRVSubtarget to represent the environment. It can be either OpenCL, Kernel or Unknown. If the triple is explicit, we can directly set it at the creation of the SPIRVSubtarget, otherwise we just leave it unknown until we find other information that can help us set the environment. For now, the only other information we use to set the environment is hlsl.shader attribute at SPIRV::ExecutionModel::ExecutionModel getExecutionModel(const SPIRVSubtarget &STI, const Function &F). Going forward we should consider also specific instructions that are Kernel-exclusive or Shader-exclusive.

llvmbot · 2025-03-04T11:39:28Z

@llvm/pr-subscribers-backend-spir-v

Author: Marcos Maronas (maarquitos14)

Changes

The test is intended to specifically test spirv-friendly builtins, so change the function name to spirv-friendly format.

Full diff: https://github.com/llvm/llvm-project/pull/129689.diff

1 Files Affected:

(modified) llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions/spirv-friendly_extended_bit_ops.ll (+2-2)

diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions/spirv-friendly_extended_bit_ops.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions/spirv-friendly_extended_bit_ops.ll
index e49a55956aff0..c93a4ad1bf1c9 100644
--- a/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions/spirv-friendly_extended_bit_ops.ll
+++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_KHR_bit_instructions/spirv-friendly_extended_bit_ops.ll
@@ -76,12 +76,12 @@ declare spir_func <8 x i8> @_Z24__spirv_BitFieldUExtractDv8_hjj(<8 x i8>, i32, i
 ; }
 define spir_kernel void @testBitReverse_SPIRVFriendly(<4 x i64> %b, ptr addrspace(1) nocapture align 32 %res) #3 {
 entry:
-  %call = call <4 x i64> @llvm.bitreverse.v4i64(<4 x i64> %b)
+  %call = call <4 x i64> @_Z18__spirv_BitReverseDv4_l(<4 x i64> %b)
   store <4 x i64> %call, ptr addrspace(1) %res, align 32
   ret void
 }
 
-declare <4 x i64> @llvm.bitreverse.v4i64(<4 x i64>) #4
+declare <4 x i64> @_Z18__spirv_BitReverseDv4_l(<4 x i64>) #4

github-actions · 2025-04-02T15:02:47Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Keenuts

I think this is OK for me, as I don't think there are any other target than OpenCL/Vulkan for now. This can be revisited when Microsoft adds their DX/SPIRV thing

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

VyacheslavLevytskyy · 2025-04-02T15:30:59Z

I'm afraid, this looks fragile with regards to the triple. Triples of kinds, for instance, "spirv64-unknown-unknown" and "spirv64v1.6-unknown-unknown" are massively in use and nobody expects that the "-opencl" suffix change game rules.

If a user needs now to apply "-opencl" everywhere to be recognized as the user of the compute flavor of SPIR-V, I expect that pretty much everything will start to fail after this change. Probably we need a way that doesn't require updating the whole compute's set of test cases, this is the only guarantee that SYCL, Intel's XPU backend for Triton, etc. will not notice the internal change in the SPIR-V backend. This is not to mention non-Intel applications of the compute flavor of SPIR-V.

For the context it's important that isOpenCLEnv() is just a bad choice of the name for such a function, it should be renamed to something of a kind of isCompute() to reflect what it means.

VyacheslavLevytskyy · 2025-04-02T15:35:46Z

Let's add to the discussion more people: @michalpaszkowski @MrSidims @asudarsa

maarquitos14 · 2025-04-02T20:42:43Z

I'm afraid, this looks fragile with regards to the triple. Triples of kinds, for instance, "spirv64-unknown-unknown" and "spirv64v1.6-unknown-unknown" are massively in use and nobody expects that the "-opencl" suffix change game rules.

If a user needs now to apply "-opencl" everywhere to be recognized as the user of the compute flavor of SPIR-V, I expect that pretty much everything will start to fail after this change. Probably we need a way that doesn't require updating the whole compute's set of test cases, this is the only guarantee that SYCL, Intel's XPU backend for Triton, etc. will not notice the internal change in the SPIR-V backend. This is not to mention non-Intel applications of the compute flavor of SPIR-V.

I would expect that updating Clang to generate opencl part of the triple for those other projects should suffice. What do you think?

For the context it's important that isOpenCLEnv() is just a bad choice of the name for such a function, it should be renamed to something of a kind of isCompute() to reflect what it means.

I agree, and similarly, isVulkanEnv() should be renamed to isShader(), I think.

VyacheslavLevytskyy · 2025-04-03T10:37:17Z

CC: @AlexVlx

MrSidims

This patch is LGTM, but IMHO this is a case when a work shouldn't be split across components - I mean that either this PR should also include changes in clang, that are forcing OpenCL environment to be included in the triple information for OpenCL lang or the appropriate PR in clang should already exist and be linked here as we also need to have attention and feedback from the frontend folks.

fyi @svenvh

bader · 2025-04-07T20:28:03Z

llvm/lib/Target/SPIRV/SPIRVSubtarget.h

  }
-  bool isVulkanEnv() const { return TargetTriple.getArch() == Triple::spirv; }
+  bool isVulkanEnv() const { return !isOpenCLEnv(); }


I see that Vulkan is added to the triple as OSType:

llvm-project/llvm/include/llvm/TargetParser/Triple.h

Line 241 in 7aedeba

Vulkan, // Vulkan SPIR-V

SPIR target uses Vulkan OSType to detect Vulkan environment:

llvm-project/clang/lib/Basic/Targets/SPIR.h

Lines 305 to 312 in 9fdac84

assert(Triple.getArch() == llvm::Triple::spirv &&

"Invalid architecture for Logical SPIR-V.");

assert(Triple.getOS() == llvm::Triple::Vulkan &&

Triple.getVulkanVersion() != llvm::VersionTuple(0) &&

"Logical SPIR-V requires a valid Vulkan environment.");

assert(Triple.getEnvironment() >= llvm::Triple::Pixel &&

Triple.getEnvironment() <= llvm::Triple::Amplification &&

"Logical SPIR-V environment must be a valid shader stage.");

OpenCL is a bit messy.
I see one* OpenCL OSType:

llvm-project/llvm/include/llvm/TargetParser/Triple.h

Line 222 in 7aedeba

NVCL, // NVIDIA OpenCL

Environment component also defines OpenCL:

llvm-project/llvm/include/llvm/TargetParser/Triple.h

Line 300 in 7aedeba

OpenCL,

I don't know the meaning of OpenCL in the environment component, but using OS triple component makes more sense to me. This topic probably forth a separate discussion, but I would add OpenCL component to OSType rather than to environment component. I guess we can alias NVCL to the OpenCL OSType value.

* - Maybe Mesa3D can be also considered as OpenCL environment.

I don't know the meaning of OpenCL in the environment component

It was added in #78655 and seems to be just a side change. I've asked the author in the PR, what was the idea behind adding OpenCL to env, but not sure if I'll get a reply.

bader · 2025-04-07T21:13:34Z

llvm/lib/Target/SPIRV/SPIRVSubtarget.h

  }
-  bool isVulkanEnv() const { return TargetTriple.getArch() == Triple::spirv; }
+  bool isVulkanEnv() const { return !isOpenCLEnv(); }


bool isVulkanEnv() const { return !isOpenCLEnv(); }

This code says that any triple that doesn't match OpenCL environment is Vulkan environment.

I would prefer SPIR-V backend to keep existing behavior for existing triple i.e. spirv[32|64]-unknown-unknown -> OpenCL environment.

New triples:

spirv-<vendor>-vulkan - SPIR-V for Vulkan environment.

spirv[32|64]-<vendor>-opencl - SPIR-V for OpenCL environment.

One of the things that we would like to fix is the fact that Vulkan/Shader environment can also be for a physical target (spirv32, spirv64). In other words, although de facto every physical SPIRV is Compute, and every logical SPIRV is Shader, that is not a requirement from the spec, so we should be able to support logical SPIRV for Compute and physical SPIRV for Shader.

Can it be done in less intrusive way e.g. by requiring vulkan OS in the target triple?
If OS target triple is not Vulkan, the environment is set to OpenCL.
This way, we won't break the existing interface for FEs using "unknown" OS type to target OpenCL environment.

The new approach I just added shouldn't break the existing interface for FEs using "unknown" OS type to target OpenCL environment.

maarquitos14 · 2025-04-23T11:15:46Z

@Keenuts the new approach I just added makes 58 tests fail. They can all pass if we explicitly pass vulkan in the triple. I didn't look at all of them individually, but took a look at a handful of them and from my POV, there's not enough information to make them work with unknown-unknown triple. In other words, I don't see anything Shader/Vulkan specific in them so that we can infer from the module what's the environment. Therefore, I think the only way to make them pass would be explicitly setting the triple to include vulkan. What do you think? Is that acceptable?

maarquitos14 · 2025-04-29T09:34:50Z

@Keenuts friendly ping

Keenuts · 2025-04-30T16:21:14Z

@Keenuts friendly ping

Sorry, missed your notification
You mean we have tests using spirv-unknown-unknown for Vulkan? If so, yes I don't see any issues with updating those to have the explicit vulkan env bit

maarquitos14 · 2025-04-30T19:00:15Z

@Keenuts friendly ping

Sorry, missed your notification You mean we have tests using spirv-unknown-unknown for Vulkan? If so, yes I don't see any issues with updating those to have the explicit vulkan env bit

Yes, that's what I meant. I'll update those and ping again when the CI is green to make sure we are on the same page.

maarquitos14 · 2025-05-02T10:53:12Z

Don't know for others, but there is the re-request review button which will make this reappear in the https://github.com/pulls/review-requested page (vs a ping which is 1 notification in a ocean of noise)

Thanks, I have done it now. Originally I couldn't even ask for reviewers myself (@VyacheslavLevytskyy did it for me), so I assumed I could not ask for re-review either, but turns out I can.

llvm/lib/Target/SPIRV/SPIRVSubtarget.h

…endly-call

MrSidims

Thanks! I believe that this approach is good enough. Later some extra heuristics can be added, but first it seems like we need to modify clang to update triple for OpenCL.

llvm/lib/Target/SPIRV/SPIRVSubtarget.h

maarquitos14 · 2025-05-21T13:39:13Z

@Keenuts can you confirm this is good from Vulkan side of things?

Keenuts

LGTM. Just some trivial nits to flip conditions.

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp

llvm/lib/Target/SPIRV/SPIRVPrepareFunctions.cpp

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp

…endly-call

maarquitos14 · 2025-05-28T15:11:53Z

@svenvh @AlexVlx @michalpaszkowski @bader just pinging to see if you have any comments about this PR. Otherwise, as this already has 3 approvals, I'll ask gatekeepers to merge in a couple of days.

bader · 2025-05-28T15:27:02Z

llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp

@@ -532,7 +532,7 @@ void SPIRVAsmPrinter::outputExecutionMode(const Module &M) {
      Inst.addOperand(MCOperand::createImm(TypeCode));
      outputMCInst(Inst);
    }
-    if (ST->isOpenCLEnv() && !M.getNamedMetadata("spirv.ExecutionMode") &&
+    if (ST->isKernelEnv() && !M.getNamedMetadata("spirv.ExecutionMode") &&


Change how to detect OpenCL/Vulkan Env and update tests accordingly

Let me clarify the terminology.

SPIR-V specification defines OpenCL/Vulkan as environments and Kernel/Shader as capabilities.

What exactly are we detecting here: environments or capabilities?

To be honest, I don't see in the SPIR-V spec anything about environments being OpenCL/Vulkan. Anyway, I think what we are detecting here is capabilities because most uses of isKernelEnv()/isShaderEnv() are used to check if a particular feature is allowed --e.g. Constant decoration is only allowed if Kernel capability is enabled. Fundamentally, we want to know if we're dealing with graphical shaders or compute kernels.

Anyway, I think what we are detecting here is capabilities because most uses of isKernelEnv()/isShaderEnv() are used to check if a particular feature is allowed --e.g. Constant decoration is only allowed if Kernel capability is enabled. Fundamentally, we want to know if we're dealing with graphical shaders or compute kernels.

Please, make the method name clearer. E.g.

Suggested change

if (ST->isKernelEnv() && !M.getNamedMetadata("spirv.ExecutionMode") &&

if (ST->isKernel() && !M.getNamedMetadata("spirv.ExecutionMode") &&

or requiresKernelCapability().

To be honest, I don't see in the SPIR-V spec anything about environments being OpenCL/Vulkan.

Right, SPIR-V spec only says that there are separate specifications:

1.2. Execution Environment and Client API
SPIR-V is adaptable to multiple execution environments: A SPIR-V module is consumed by an execution environment, as specified by a client API. The full set of rules needed to consume SPIR-V in a particular environment comes from the combination of SPIR-V and that environment’s client API specification. The client API specifies its SPIR-V execution environment as well as extra rules, limitations, capabilities, etc. required by the form of SPIR-V it can validly consume.

OpenCL provides The OpenCL™ SPIR-V Environment Specification and Vulkan provides Vulkan Environment for SPIR-V. There are other execution environments supporting SPIR-V formats (e.g. Level Zero)

My understanding is that compiler performs additional checks for restrictions in the environment specification, which is specified as OS component of the target triple.

Would isCompute()/isShader() work for you?

I prefer using terminology from the spec to avoid ambiguity.
For instance, Vulkan uses "compute shader" term for shaders doing GPGPU computations (i.e. what we call just "Shader" in clang project) - https://vulkan-tutorial.com/Compute_Shader.
Is it okay to use isKernel/isShader?

It works for me. Let's see if any of the people that already approved disagrees: @Keenuts @VyacheslavLevytskyy @MrSidims. If no comments in the next 24 hours, I'll assume they agree and I'll go ahead and change them to isKernel/isShader.

I'm fine with isKernel/isShader (I'd be against isCompute for OpenCL as this could be confusing)

Updated in 42df23d.

…endly-call

maarquitos14 · 2025-05-31T09:25:01Z

@VyacheslavLevytskyy @michalpaszkowski can we get this merged?

llvmbot added the backend:SPIR-V label Mar 4, 2025

maarquitos14 changed the title ~~Use SPIRV-friendly builtin name.~~ [SPIRV] Use SPIRV-friendly builtin name. Mar 4, 2025

VyacheslavLevytskyy self-requested a review March 4, 2025 11:46

maarquitos14 force-pushed the maronas/use-spirv-friendly-call branch from b8daa27 to 11e5b6f Compare April 2, 2025 14:59

maarquitos14 changed the title ~~[SPIRV] Use SPIRV-friendly builtin name.~~ [SPIRV] Change how to detect OpenCL/Vulkan Env and update tests accordingly. Apr 2, 2025

maarquitos14 marked this pull request as draft April 2, 2025 15:04

Keenuts approved these changes Apr 2, 2025

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp Outdated Show resolved Hide resolved

MrSidims requested review from bader, svenvh and AlexVlx April 4, 2025 11:07

MrSidims reviewed Apr 4, 2025

View reviewed changes

VyacheslavLevytskyy requested a review from michalpaszkowski April 7, 2025 08:19

bader reviewed Apr 7, 2025

View reviewed changes

maarquitos14 force-pushed the maronas/use-spirv-friendly-call branch from 11e5b6f to eef5045 Compare April 23, 2025 11:01

marcos.maronas and others added 4 commits April 24, 2025 11:51

Dissociate Logical/Physical from OpenCL/Vulkan env.

bbf7b42

Extra change to complete dissociation.

f6bf74b

Use entry points to help determine the env.

4c7bbc2

Explicitly add vulkan to triple of failing tests.

ecb4f79

Fix clang-format issue.

1a4e754

maarquitos14 requested a review from Keenuts May 2, 2025 10:51

bader reviewed May 2, 2025

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVSubtarget.h Outdated Show resolved Hide resolved

maarquitos14 added 2 commits May 14, 2025 11:19

Merge remote-tracking branch 'origin/main' into maronas/use-spirv-fri…

3221f7e

…endly-call

Address code review feedback.

0cb18d0

maarquitos14 requested a review from bader May 14, 2025 14:09

maarquitos14 added 6 commits May 14, 2025 16:57

Merge remote-tracking branch 'origin/main' into maronas/use-spirv-fri…

4d84546

…endly-call

Fix build issues.

7a0be57

Fix typo.

70835b9

Fix test failure.

afa72c1

Merge remote-tracking branch 'origin/main' into maronas/use-spirv-fri…

b7879b5

…endly-call

Fix test failure.

85d600f

MrSidims approved these changes May 15, 2025

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVSubtarget.h Outdated Show resolved Hide resolved

bader mentioned this pull request May 15, 2025

[SPIRV] Addition of @llvm.lround.* and @llvm.llround.* intrinsic #129240

Open

VyacheslavLevytskyy approved these changes May 19, 2025

View reviewed changes

Keenuts approved these changes May 27, 2025

View reviewed changes

maarquitos14 added 4 commits May 27, 2025 16:56

Merge remote-tracking branch 'origin/main' into maronas/use-spirv-fri…

5513335

…endly-call

Address code review feedback.

61f2abd

Address more code review feedback.

a36faa4

Turn assertion into fatal error.

fcc219a

Keenuts approved these changes May 28, 2025

View reviewed changes

bader requested changes May 28, 2025

View reviewed changes

maarquitos14 added 3 commits May 30, 2025 13:02

Replace is[Kernel|Shader]Env with is[Kernel|Shader].

42df23d

Merge remote-tracking branch 'origin/main' into maronas/use-spirv-fri…

dd530b9

…endly-call

Fix test failure.

248c5e6

maarquitos14 requested a review from bader May 30, 2025 19:22

bader approved these changes May 30, 2025

View reviewed changes

	assert(Triple.getArch() == llvm::Triple::spirv &&
	"Invalid architecture for Logical SPIR-V.");
	assert(Triple.getOS() == llvm::Triple::Vulkan &&
	Triple.getVulkanVersion() != llvm::VersionTuple(0) &&
	"Logical SPIR-V requires a valid Vulkan environment.");
	assert(Triple.getEnvironment() >= llvm::Triple::Pixel &&
	Triple.getEnvironment() <= llvm::Triple::Amplification &&
	"Logical SPIR-V environment must be a valid shader stage.");

	if (ST->isKernelEnv() && !M.getNamedMetadata("spirv.ExecutionMode") &&
	if (ST->isKernel() && !M.getNamedMetadata("spirv.ExecutionMode") &&

[SPIRV] Change how to detect OpenCL/Vulkan Env and update tests accordingly. #129689

Are you sure you want to change the base?

[SPIRV] Change how to detect OpenCL/Vulkan Env and update tests accordingly. #129689

Uh oh!

Conversation

maarquitos14 commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 4, 2025

Uh oh!

github-actions bot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keenuts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VyacheslavLevytskyy commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

VyacheslavLevytskyy commented Apr 2, 2025

Uh oh!

maarquitos14 commented Apr 2, 2025

Uh oh!

VyacheslavLevytskyy commented Apr 3, 2025

Uh oh!

MrSidims left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maarquitos14 commented Apr 23, 2025

Uh oh!

maarquitos14 commented Apr 29, 2025

Uh oh!

Keenuts commented Apr 30, 2025

Uh oh!

maarquitos14 commented Apr 30, 2025

Uh oh!

maarquitos14 commented May 2, 2025

Uh oh!

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maarquitos14 commented May 21, 2025

Uh oh!

Keenuts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maarquitos14 commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maarquitos14 commented Mar 4, 2025 •

edited

Loading

github-actions bot commented Apr 2, 2025 •

edited

Loading

VyacheslavLevytskyy commented Apr 2, 2025 •

edited

Loading

MrSidims left a comment •

edited

Loading