[SPIRV] Requires SCFG for Vulkan's SPIR-V

[Keenuts]

This commit sets the target requiresSCFG to true when building Vulkan. This is a first step toward bringing structured control flow to this backend.

Now that we have this flag, we can disable a first problematic transformation: removal of branches when it falls through. This is required in the SPIR-V specification:
- Each basic block MUST end with a branch instruction.
This means it is not valid to just fall-through.

note: this commit does not generate valid CFG for graphical SPIR-V. Merge headers are missing, and outside of this specific edge case, we don't generate structured SCFG. Just a first independent step.

[llvmbot]

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

@llvm/pr-subscribers-llvm-globalisel

Changes

This commit sets the target requiresSCFG to true when building Vulkan. This is a first step toward bringing structured control flow to this backend.

Now that we have this flag, we can disable a first problematic transformation: removal of branches when it falls through. This is required in the SPIR-V specification:
- Each basic block MUST end with a branch instruction.
This means it is not valid to just fall-through.

note: this commit does not generate valid CFG for graphical SPIR-V. Merge headers are missing, and outside of this specific edge case, we don't generate structured SCFG. Just a first independent step.

---

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

3 Files Affected:

• (modified) llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp (+3)
• (modified) llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp (+1-1)
• (added) llvm/test/CodeGen/SPIRV/scfg-basic.ll (+43)

diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 764567ac7baada6..39278971782a1de 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -578,7 +578,10 @@ bool IRTranslator::translateBr(const User &U, MachineIRBuilder &MIRBuilder) {
 
   if (BrInst.isUnconditional()) {
     // If the unconditional target is the layout successor, fallthrough.
+    // Except if the target requires SCFGs. In such case, we should keep the IR
+    // branch.
     if (OptLevel == CodeGenOptLevel::None ||
+        MF->getTarget().requiresStructuredCFG() ||
         !CurMBB.isLayoutSuccessor(Succ0MBB))
       MIRBuilder.buildBr(*Succ0MBB);
 
diff --git a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
index 14dd429b451910d..dcd7f3c95fded8a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
@@ -82,7 +82,7 @@ SPIRVTargetMachine::SPIRVTargetMachine(const Target &T, const Triple &TT,
   setGlobalISel(true);
   setFastISel(false);
   setO0WantsFastISel(false);
-  setRequiresStructuredCFG(false);
+  setRequiresStructuredCFG(Subtarget.isVulkanEnv());
 }
 
 namespace {
diff --git a/llvm/test/CodeGen/SPIRV/scfg-basic.ll b/llvm/test/CodeGen/SPIRV/scfg-basic.ll
new file mode 100644
index 000000000000000..2fd42297999c858
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/scfg-basic.ll
@@ -0,0 +1,43 @@
+; RUN: llc -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s
+
+; CHECK-DAG:      [[uint:%[0-9]+]] = OpTypeInt 32 0
+; CHECK-DAG:      [[bool:%[0-9]+]] = OpTypeBool
+; CHECK-DAG:  [[ptr_uint:%[0-9]+]] = OpTypePointer Function [[uint]]
+; CHECK-DAG:    [[uint_0:%[0-9]+]] = OpConstant [[uint]] 0
+; CHECK-DAG:    [[uint_10:%[0-9]+]] = OpConstant [[uint]] 10
+; CHECK-DAG:    [[uint_20:%[0-9]+]] = OpConstant [[uint]] 20
+
+define void @main() #1 {
+  %input = alloca i32, align 4
+  %output = alloca i32, align 4
+; CHECK: [[input:%[0-9]+]] = OpVariable [[ptr_uint]] Function
+; CHECK: [[output:%[0-9]+]] = OpVariable [[ptr_uint]] Function
+
+  %1 = load i32, i32* %input, align 4
+  %2 = icmp ne i32 %1, 0
+  br i1 %2, label %true, label %false
+; CHECK:  [[tmp:%[0-9]+]] = OpLoad [[uint]] [[input]]
+; CHECK: [[cond:%[0-9]+]] = OpINotEqual [[bool]] [[tmp]] [[uint_0]]
+; CHECK:                    OpBranchConditional [[cond]] [[true:%[0-9]+]] [[false:%[0-9]+]]
+
+true:
+  store i32 10, i32* %output, align 4
+  br label %merge
+; CHECK: [[true]] = OpLabel
+; CHECK:            OpStore [[output]] [[uint_10]]
+; CHECK:            OpBranch [[merge:%[0-9]+]]
+
+false:
+  store i32 20, i32* %output, align 4
+  br label %merge
+; CHECK: [[false]] = OpLabel
+; CHECK:            OpStore [[output]] [[uint_20]]
+; CHECK:            OpBranch [[merge]]
+
+merge:
+; CHECK: [[merge]] = OpLabel
+; CHECK:             OpReturn
+  ret void
+}
+
+attributes #1 = { "hlsl.numthreads"="4,8,16" "hlsl.shader"="compute" convergent }

[Keenuts]

TBH, I am not convinced this is the right solution.
Seems like having a condition here controlling this generic behavior while this is only required for SPIR-V seems wrong.
But checking for a specific target in this code also feels weird.

Leaving this as draft for now, until we figured out a path forward for a structurizer.