[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

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- llvm/lib/Target/SPIRV/SPIRVAsmPrinter.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 llvm/lib/Target/SPIRV/SPIRVSubtarget.h llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 39cdf04b6..d1342cb46 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -2205,11 +2205,12 @@ bool SPIRVInstructionSelector::selectWaveOpInst(Register ResVReg,
   MachineBasicBlock &BB = *I.getParent();
   SPIRVType *IntTy = GR.getOrCreateSPIRVIntegerType(32, I, TII);
 
-  auto BMI = BuildMI(BB, I, I.getDebugLoc(), TII.get(Opcode))
-                 .addDef(ResVReg)
-                 .addUse(GR.getSPIRVTypeID(ResType))
-                 .addUse(GR.getOrCreateConstInt(SPIRV::Scope::Subgroup, I,
-                                                IntTy, TII, !STI.isLogicalSPIRV()));
+  auto BMI =
+      BuildMI(BB, I, I.getDebugLoc(), TII.get(Opcode))
+          .addDef(ResVReg)
+          .addUse(GR.getSPIRVTypeID(ResType))
+          .addUse(GR.getOrCreateConstInt(SPIRV::Scope::Subgroup, I, IntTy, TII,
+                                         !STI.isLogicalSPIRV()));
 
   for (unsigned J = 2; J < I.getNumOperands(); J++) {
     BMI.addUse(I.getOperand(J).getReg());

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

Keenuts · 2025-04-02T15:06:33Z

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp

-        "This entry point lacks mandatory hlsl.shader attribute.");
+        "This entry point lacks mandatory hlsl.shader attribute.", false);
  }

  const auto value = attribute.getValueAsString();
  if (value == "compute")
    return SPIRV::ExecutionModel::GLCompute;

-  report_fatal_error("This HLSL entry point is not supported by this backend.");


unrelated no?

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?

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

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

marcos.maronas and others added 3 commits April 23, 2025 13:00

Dissociate Logical/Physical from OpenCL/Vulkan env.

c956e4b

Extra change to complete dissociation.

f87ac68

Use entry points to help determine the env.

eef5045

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maarquitos14 commented Mar 4, 2025 •

edited

Loading

llvmbot commented Mar 4, 2025

github-actions bot commented Apr 2, 2025 •

edited

Loading

Keenuts left a comment

Keenuts Apr 2, 2025

VyacheslavLevytskyy commented Apr 2, 2025 •

edited

Loading

VyacheslavLevytskyy commented Apr 2, 2025

maarquitos14 commented Apr 2, 2025

VyacheslavLevytskyy commented Apr 3, 2025

MrSidims left a comment •

edited

Loading

bader Apr 7, 2025

MrSidims Apr 7, 2025

bader Apr 7, 2025

maarquitos14 Apr 9, 2025

bader Apr 9, 2025

maarquitos14 Apr 23, 2025

maarquitos14 commented Apr 23, 2025

	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.");

[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

Conversation

maarquitos14 commented Mar 4, 2025 • edited Loading

llvmbot commented Mar 4, 2025

github-actions bot commented Apr 2, 2025 • edited Loading

Keenuts left a comment

Choose a reason for hiding this comment

Keenuts Apr 2, 2025

Choose a reason for hiding this comment

VyacheslavLevytskyy commented Apr 2, 2025 • edited Loading

VyacheslavLevytskyy commented Apr 2, 2025

maarquitos14 commented Apr 2, 2025

VyacheslavLevytskyy commented Apr 3, 2025

MrSidims left a comment • edited Loading

Choose a reason for hiding this comment

bader Apr 7, 2025

Choose a reason for hiding this comment

MrSidims Apr 7, 2025

Choose a reason for hiding this comment

bader Apr 7, 2025

Choose a reason for hiding this comment

maarquitos14 Apr 9, 2025

Choose a reason for hiding this comment

bader Apr 9, 2025

Choose a reason for hiding this comment

maarquitos14 Apr 23, 2025

Choose a reason for hiding this comment

maarquitos14 commented Apr 23, 2025

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