Skip to content
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

[SPIR-V] Don't replace spirv.Type with spv_track_constant #134448

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cassiebeckley
Copy link
Member

Fixes #134417

@llvmbot
Copy link
Member

llvmbot commented Apr 4, 2025

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

Author: Cassandra Beckley (cassiebeckley)

Changes

Fixes #134417


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

2 Files Affected:

  • (modified) llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp (+3-11)
  • (added) llvm/test/CodeGen/SPIRV/inline/type.undef.ll (+17)
diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index 0067d2400529a..064e9c7074750 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -2062,21 +2062,13 @@ void SPIRVEmitIntrinsics::processInstrAfterVisit(Instruction *I,
     }
     Type *OpTy = Op->getType();
     Value *OpTyVal = Op;
-    if (OpTy->isTargetExtTy())
-      OpTyVal = getNormalizedPoisonValue(OpTy);
     Type *OpElemTy = GR->findDeducedElementType(Op);
     Value *NewOp = Op;
-    if (OpTy->isTargetExtTy()) {
+    TargetExtType *TargetTy = dyn_cast<TargetExtType>(OpTy);
+    if (TargetTy && TargetTy->getName() != "spirv.Type") {
+      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..1b8a8428fc74c
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/inline/type.undef.ll
@@ -0,0 +1,17 @@
+; 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:
+; CHECK-DAG: [[undef:%[0-9]+]] = OpUndef [[int_t]]
+; CHECK-DAG: OpReturnValue [[undef]]
+  ret %int_t undef
+}

Copy link

github-actions bot commented Apr 4, 2025

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:
git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 'HEAD~1' HEAD llvm/test/CodeGen/SPIRV/inline/type.undef.ll llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp

The following files introduce new uses of undef:

  • llvm/test/CodeGen/SPIRV/inline/type.undef.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

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.

[SPIR-V] Returning undef from a function with a spirv.Type return type crashes
2 participants