Skip to content

[SPIR-V] Ensure that OpExtInst instructions generated by NonSemantic_Shader_DebugInfo_100 are not mixed up with other OpExtInst instructions #107007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

VyacheslavLevytskyy
Copy link
Contributor

This PR is to ensure that OpExtInst instructions generated by NonSemantic_Shader_DebugInfo_100 are not mixed up with other OpExtInst instructions.

Original implementation (#97558) has introduced an issue by moving OpExtInst instruction with the 3rd operand equal to DebugSource (value 35) or DebugCompilationUnit (value 1) even if OpExtInst is not generated by NonSemantic_Shader_DebugInfo_100 implementation code.

The reproducer is attached as a new test case. The code of the test case reproduces the issue, because "lgamma" has the same code (35) inside OpenCL_std as DebugSource inside NonSemantic_Shader_DebugInfo_100.

…bugInfo_100 are not mixed up with other OpExtInst instructions
@llvmbot
Copy link
Member

llvmbot commented Sep 2, 2024

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

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

This PR is to ensure that OpExtInst instructions generated by NonSemantic_Shader_DebugInfo_100 are not mixed up with other OpExtInst instructions.

Original implementation (#97558) has introduced an issue by moving OpExtInst instruction with the 3rd operand equal to DebugSource (value 35) or DebugCompilationUnit (value 1) even if OpExtInst is not generated by NonSemantic_Shader_DebugInfo_100 implementation code.

The reproducer is attached as a new test case. The code of the test case reproduces the issue, because "lgamma" has the same code (35) inside OpenCL_std as DebugSource inside NonSemantic_Shader_DebugInfo_100.


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

2 Files Affected:

  • (modified) llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp (+4-1)
  • (added) llvm/test/CodeGen/SPIRV/debug-info/no-misplaced-opextinst.ll (+36)
diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
index a2fcfc636e3684..df42b6de193bda 100644
--- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
@@ -428,7 +428,10 @@ void SPIRVModuleAnalysis::processOtherInstrs(const Module &M) {
         const unsigned OpCode = MI.getOpcode();
         if (OpCode == SPIRV::OpString) {
           collectOtherInstr(MI, MAI, SPIRV::MB_DebugStrings, IS);
-        } else if (OpCode == SPIRV::OpExtInst) {
+        } else if (OpCode == SPIRV::OpExtInst && MI.getOperand(2).isImm() &&
+                   MI.getOperand(2).getImm() ==
+                       SPIRV::InstructionSet::
+                           NonSemantic_Shader_DebugInfo_100) {
           MachineOperand Ins = MI.getOperand(3);
           namespace NS = SPIRV::NonSemanticExtInst;
           static constexpr int64_t GlobalNonSemanticDITy[] = {
diff --git a/llvm/test/CodeGen/SPIRV/debug-info/no-misplaced-opextinst.ll b/llvm/test/CodeGen/SPIRV/debug-info/no-misplaced-opextinst.ll
new file mode 100644
index 00000000000000..b3d202505d1226
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/debug-info/no-misplaced-opextinst.ll
@@ -0,0 +1,36 @@
+; This test is to ensure that OpExtInst generated by NonSemantic_Shader_DebugInfo_100
+; are not mixed up with other OpExtInst instructions.
+; The code of the test is a reproducer, because "lgamma" has the same code (35)
+; inside OpenCL_std as DebugSource inside NonSemantic_Shader_DebugInfo_100.
+
+; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+
+; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+
+; CHECK: %[[#Ocl:]] = OpExtInstImport "OpenCL.std"
+; CHECK: OpName %[[#Fun:]] "__devicelib_lgammaf"
+; CHECK: %[[#Fun]] = OpFunction %[[#]] None %[[#]]
+; CHECK: OpFunctionParameter
+; CHECK: %[[#]] = OpExtInst %[[#]] %[[#Ocl]] lgamma %[[#]]
+
+define weak_odr dso_local spir_kernel void @foo() {
+entry:
+  %r = tail call spir_func noundef float @lgammaf(float noundef 0x7FF8000000000000)
+  ret void
+}
+
+define weak dso_local spir_func float @lgammaf(float noundef %x) {
+entry:
+  %call = tail call spir_func float @__devicelib_lgammaf(float noundef %x)
+  ret float %call
+}
+
+define weak dso_local spir_func float @__devicelib_lgammaf(float noundef %x) {
+entry:
+  %call = tail call spir_func noundef float @_Z18__spirv_ocl_lgammaf(float noundef %x)
+  ret float %call
+}
+
+declare dso_local spir_func noundef float @_Z18__spirv_ocl_lgammaf(float noundef)

@VyacheslavLevytskyy
Copy link
Contributor Author

VyacheslavLevytskyy commented Sep 2, 2024

@bogner @farzonl JFYI, to save time if you see this issue, it would appear for Round and Modf

@VyacheslavLevytskyy VyacheslavLevytskyy merged commit 4f403e8 into llvm:main Sep 3, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants