[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.