[SPIR-V] Consistent handling of TargetExtTypes in emit-intrinsics

[cassiebeckley]

TargetExtType values are replaced with calls to llvm.spv.track.constant, with a poison value, but llvm.spv.assign.type was called with their original value. This PR updates the assign.type call to be consistent with the track.constant call.

Fixes #134417.

[llvmbot]

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

Author: Cassandra Beckley (cassiebeckley)

Changes

TargetExtType values are replaced with calls to llvm.spv.track.constant, with a poison value, but llvm.spv.assign.type was called with their original value. This PR updates the assign.type call to be consistent with the track.constant call.

Fixes #134417.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp (+7-13)
• (added) llvm/test/CodeGen/SPIRV/inline/type.undef.ll (+20)

diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index 0067d2400529a..b72de93d041f7 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -1947,10 +1947,14 @@ void SPIRVEmitIntrinsics::insertAssignTypeIntrs(Instruction *I,
           GR->buildAssignPtr(B, ElemTy ? ElemTy : deduceElementType(Op, true),
                              Op);
         } else {
+          Value *OpTyVal = Op;
+          if (OpTy->isTargetExtTy()) {
+            OpTyVal = getNormalizedPoisonValue(OpTy);
+          }
           CallInst *AssignCI =
               buildIntrWithMD(Intrinsic::spv_assign_type, {OpTy},
-                              getNormalizedPoisonValue(OpTy), Op, {}, B);
-          GR->addAssignPtrTypeInstr(Op, AssignCI);
+                              getNormalizedPoisonValue(OpTy), OpTyVal, {}, B);
+          GR->addAssignPtrTypeInstr(OpTyVal, AssignCI);
         }
       }
     }
@@ -2061,22 +2065,12 @@ void SPIRVEmitIntrinsics::processInstrAfterVisit(Instruction *I,
       BPrepared = true;
     }
     Type *OpTy = Op->getType();
-    Value *OpTyVal = Op;
-    if (OpTy->isTargetExtTy())
-      OpTyVal = getNormalizedPoisonValue(OpTy);
     Type *OpElemTy = GR->findDeducedElementType(Op);
     Value *NewOp = Op;
     if (OpTy->isTargetExtTy()) {
+      Value *OpTyVal = getNormalizedPoisonValue(OpTy);
       NewOp = buildIntrWithMD(Intrinsic::spv_track_constant,
                               {OpTy, OpTyVal->getType()}, Op, OpTyVal, {}, B);
-      if (isPointerTy(OpTy)) {
-        if (OpElemTy) {
-          GR->buildAssignPtr(B, OpElemTy, NewOp);
-        } else {
-          insertTodoType(NewOp);
-          GR->buildAssignPtr(B, OpTy, NewOp);
-        }
-      }
     }
     if (!IsConstComposite && isPointerTy(OpTy) && OpElemTy != nullptr &&
         OpElemTy != IntegerType::getInt8Ty(I->getContext())) {
diff --git a/llvm/test/CodeGen/SPIRV/inline/type.undef.ll b/llvm/test/CodeGen/SPIRV/inline/type.undef.ll
new file mode 100644
index 0000000000000..8cb90007c9aba
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/inline/type.undef.ll
@@ -0,0 +1,20 @@
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-unknown %s -o - | spirv-as - -o - | spirv-val %}
+
+%literal_32 = type target("spirv.Literal", 32)
+%literal_true = type target("spirv.Literal", 1)
+
+; CHECK-DAG: OpUnknown(21, 4) [[int_t:%[0-9]+]] 32 1
+%int_t = type target("spirv.Type", %literal_32, %literal_true, 21, 4, 32)
+
+; CHECK-DAG: {{%[0-9]+}} = OpTypeFunction [[int_t]]
+define %int_t @foo() {
+entry:
+  %v = alloca %int_t
+  %i = load %int_t, ptr %v
+
+; CHECK-DAG: [[i:%[0-9]+]] = OpUndef [[int_t]]
+; CHECK-DAG: OpReturnValue [[i]]
+  ret %int_t %i
+}

[VyacheslavLevytskyy]

Let's please document this explicitly in comments.

A wider context is that in #130605 we have started elimination of spv_track_constant intrinsics, and this is a work in progress. I understand this change and its need to resolve the issue, however, it introduces a hidden relations between this point in the code and SPIRVEmitIntrinsics::processInstrAfterVisit() where Intrinsic::spv_track_constant is inserted. This create a pitfall for anyone who will try to untangle type inference logic from emit-intrinsic logic.

I think it's ok to introduce this change, but please add a longer comment here and in SPIRVEmitIntrinsics::processInstrAfterVisit(), referencing another occurrence of the spv_track_constant/spv_assign_type pair implicitly linked by isTargetExtTy(), so that we do not forget to address this after the prospective rework of type inference vs. emit-intrinsic starts.

[github-actions]

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