Skip to content

Conversation

@sarnex
Copy link
Member

@sarnex sarnex commented Aug 27, 2025

There should not be a comma between the operands.
spirv-val errors on it, and llvm-spirv doesn't emit it.

@sarnex sarnex marked this pull request as ready for review August 27, 2025 19:44
@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2025

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

Author: Nick Sarnie (sarnex)

Changes

There should not be a comma between the operands.
The spec says it shouldn't be there, spirv-val errors on it, and llvm-spirv doesn't emit it.


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

2 Files Affected:

  • (modified) llvm/lib/Target/SPIRV/SPIRVInstrInfo.td (+2-2)
  • (modified) llvm/test/CodeGen/SPIRV/llvm-intrinsics/lifetime.ll (+6-6)
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
index f0b938d681dba..8d10cd0ffb3dd 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
@@ -637,8 +637,8 @@ let isReturn = 1, hasDelaySlot = 0, isBarrier = 0, isTerminator = 1, isNotDuplic
   def OpReturnValue: Op<254, (outs), (ins ID:$ret), "OpReturnValue $ret">;
   def OpUnreachable: SimpleOp<"OpUnreachable", 255>;
 }
-def OpLifetimeStart: Op<256, (outs), (ins ID:$ptr, i32imm:$sz), "OpLifetimeStart $ptr, $sz">;
-def OpLifetimeStop: Op<257, (outs), (ins ID:$ptr, i32imm:$sz), "OpLifetimeStop $ptr, $sz">;
+def OpLifetimeStart: Op<256, (outs), (ins ID:$ptr, i32imm:$sz), "OpLifetimeStart $ptr $sz">;
+def OpLifetimeStop: Op<257, (outs), (ins ID:$ptr, i32imm:$sz), "OpLifetimeStop $ptr $sz">;
 def OpDemoteToHelperInvocation: SimpleOp<"OpDemoteToHelperInvocation", 5380>;
 
 // 3.42.18 Atomic Instructions
diff --git a/llvm/test/CodeGen/SPIRV/llvm-intrinsics/lifetime.ll b/llvm/test/CodeGen/SPIRV/llvm-intrinsics/lifetime.ll
index 3d93eca72aaed..f83cd8ad1969c 100644
--- a/llvm/test/CodeGen/SPIRV/llvm-intrinsics/lifetime.ll
+++ b/llvm/test/CodeGen/SPIRV/llvm-intrinsics/lifetime.ll
@@ -17,11 +17,11 @@
 ; CL:      OpFunction
 ; CL:      %[[#FooVar:]] = OpVariable
 ; CL-NEXT: %[[#Casted1:]] = OpBitcast %[[#PtrChar]] %[[#FooVar]]
-; CL-NEXT: OpLifetimeStart %[[#Casted1]], 16
+; CL-NEXT: OpLifetimeStart %[[#Casted1]] 16
 ; CL-NEXT: OpBitcast
 ; CL-NEXT: OpInBoundsPtrAccessChain
 ; CL-NEXT: %[[#Casted2:]] = OpBitcast %[[#PtrChar]] %[[#FooVar]]
-; CL-NEXT: OpLifetimeStop %[[#Casted2]], 16
+; CL-NEXT: OpLifetimeStop %[[#Casted2]] 16
 
 ; VK:      OpFunction
 ; VK:      %[[#FooVar:]] = OpVariable
@@ -38,11 +38,11 @@ define spir_func void @foo(ptr noundef byval(%tprange) align 8 %_arg_UserRange)
 ; CL: OpFunction
 ; CL: %[[#BarVar:]] = OpVariable
 ; CL-NEXT: %[[#Casted1:]] = OpBitcast %[[#PtrChar]] %[[#BarVar]]
-; CL-NEXT: OpLifetimeStart %[[#Casted1]], 16
+; CL-NEXT: OpLifetimeStart %[[#Casted1]] 16
 ; CL-NEXT: OpBitcast
 ; CL-NEXT: OpInBoundsPtrAccessChain
 ; CL-NEXT: %[[#Casted2:]] = OpBitcast %[[#PtrChar]] %[[#BarVar]]
-; CL-NEXT: OpLifetimeStop %[[#Casted2]], 16
+; CL-NEXT: OpLifetimeStop %[[#Casted2]] 16
 
 ; VK:      OpFunction
 ; VK:      %[[#BarVar:]] = OpVariable
@@ -58,9 +58,9 @@ define spir_func void @bar(ptr noundef byval(%tprange) align 8 %_arg_UserRange)
 
 ; CL: OpFunction
 ; CL: %[[#TestVar:]] = OpVariable
-; CL-NEXT: OpLifetimeStart %[[#TestVar]], 1
+; CL-NEXT: OpLifetimeStart %[[#TestVar]] 1
 ; CL-NEXT: OpInBoundsPtrAccessChain
-; CL-NEXT: OpLifetimeStop %[[#TestVar]], 1
+; CL-NEXT: OpLifetimeStop %[[#TestVar]] 1
 
 ; VK:      OpFunction
 ; VK:      %[[#Test:]] = OpVariable

Copy link

@Naghasan Naghasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says it shouldn't be there

just FYI: the spec doesn't define the textual form, spirv-tools' format is derived from the example in the spec but it is non-normative.

@sarnex
Copy link
Member Author

sarnex commented Aug 28, 2025

just FYI: the spec doesn't define the textual form, spirv-tools' format is derived from the example in the spec but it is non-normative.

Ah thanks, I'll remove that from the description

@sarnex sarnex merged commit 12def78 into llvm:main Aug 28, 2025
14 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.

4 participants