[SPIR-V] Ensure that internal intrinsic functions for PHI's operand are inserted at the correct positions

[VyacheslavLevytskyy]

This PR is to ensure that internal intrinsic functions for PHI's operand are inserted at the correct positions and don't break rules of instruction domination and PHI nodes grouping at top of basic block.

[llvmbot]

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

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

This PR is to ensure that internal intrinsic functions for PHI's operand are inserted at the correct positions and don't break rules of instruction domination and PHI nodes grouping at top of basic block.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp (+2-2)
• (added) llvm/test/CodeGen/SPIRV/phi-insert-point.ll (+59)

diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index c00066f5dca62..d90249f7c2e63 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -174,7 +174,7 @@ static bool isAggrToReplace(const Value *V) {
 
 static void setInsertPointSkippingPhis(IRBuilder<> &B, Instruction *I) {
   if (isa<PHINode>(I))
-    B.SetInsertPoint(I->getParent(), I->getParent()->getFirstInsertionPt());
+    B.SetInsertPoint(I->getParent()->getFirstNonPHIOrDbgOrAlloca());
   else
     B.SetInsertPoint(I);
 }
@@ -491,7 +491,7 @@ void SPIRVEmitIntrinsics::deduceOperandElementType(Instruction *I) {
     if (Instruction *User = dyn_cast<Instruction>(Op->use_begin()->get()))
       setInsertPointSkippingPhis(B, User->getNextNode());
     else
-      B.SetInsertPoint(I);
+      setInsertPointSkippingPhis(B, I);
     Value *OpTyVal = Constant::getNullValue(KnownElemTy);
     Type *OpTy = Op->getType();
     if (!Ty) {
diff --git a/llvm/test/CodeGen/SPIRV/phi-insert-point.ll b/llvm/test/CodeGen/SPIRV/phi-insert-point.ll
new file mode 100644
index 0000000000000..41cc514f4dbae
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/phi-insert-point.ll
@@ -0,0 +1,59 @@
+; The goal of the test is to check that internal intrinsic functions for PHI's
+; operand are inserted at the correct positions, and don't break rules of
+; instruction domination and PHI nodes grouping at top of basic block.
+
+; 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 %}
+
+; CHECK-DAG: OpName %[[#Foo:]] "foo"
+; CHECK-DAG: OpName %[[#Bar:]] "bar"
+; CHECK: %[[#Foo]] = OpFunction
+; CHECK: OpPhi
+; CHECK-NEXT: OpPhi
+; CHECK-NEXT: OpPhi
+; CHECK-NEXT: OpPhi
+; CHECK: %[[#Bar]] = OpFunction
+; CHECK: OpPhi
+; CHECK-NEXT: OpPhi
+; CHECK-NEXT: OpPhi
+; CHECK-NEXT: OpPhi
+
+%struct = type { i64, i64 }
+
+define spir_kernel void @foo(i64 %arg_val, ptr addrspace(4) byval(%struct) %arg_ptr) {
+entry:
+  %fl = icmp eq i64 %arg_val, 0
+  br i1 %fl, label %ok, label %err
+
+err:
+  br label %ok
+
+ok:
+  %r1 = phi i64 [ undef, %err ], [ %arg_val, %entry ]
+  %r2 = phi i64 [ undef, %err ], [ %arg_val, %entry ]
+  %r3 = phi ptr addrspace(4) [ undef, %err ], [ %arg_ptr, %entry ]
+  %r4 = phi ptr addrspace(4) [ undef, %err ], [ %arg_ptr, %entry ]
+  br label %exit
+
+exit:
+  ret void
+}
+
+define spir_kernel void @bar(i64 %arg_val, i64 %arg_val_def, ptr addrspace(4) byval(%struct) %arg_ptr, ptr addrspace(4) %arg_ptr_def) {
+entry:
+  %fl = icmp eq i64 %arg_val, 0
+  br i1 %fl, label %ok, label %err
+
+err:
+  br label %ok
+
+ok:
+  %r1 = phi i64 [ %arg_val_def, %err ], [ %arg_val, %entry ]
+  %r2 = phi i64 [ %arg_val_def, %err ], [ %arg_val, %entry ]
+  %r3 = phi ptr addrspace(4) [ %arg_ptr_def, %err ], [ %arg_ptr, %entry ]
+  %r4 = phi ptr addrspace(4) [ %arg_ptr_def, %err ], [ %arg_ptr, %entry ]
+  br label %exit
+
+exit:
+  ret void
+}

[efriedma-quic]

Maybe you want getInsertionPointAfterDef()?

The exact way you write it doesn't really matter much if you don't have exception-handling, though.

[VyacheslavLevytskyy]

Not exactly, it's a bit different case. But thank you very much for the pointer, getInsertionPointAfterDef() and its applications were worth a study.