[HLSL] Adding Flatten and Branch if attributes #116331
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-clang-codegen Author: None (joaosaffran) Changes
Closes #70112 Patch is 28.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116331.diff 17 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index b3c357ec906a23..6d3e07ce83100b 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4302,6 +4302,16 @@ def HLSLLoopHint: StmtAttr {
let Documentation = [HLSLLoopHintDocs, HLSLUnrollHintDocs];
}
+def HLSLBranchHint: StmtAttr {
+ /// [branch]
+ /// [flatten]
+ let Spellings = [Microsoft<"branch">, Microsoft<"flatten">];
+ let Subjects = SubjectList<[IfStmt],
+ ErrorDiag, "'if' statements">;
+ let LangOpts = [HLSL];
+ let Documentation = [InternalOnly];
+}
+
def CapturedRecord : InheritableAttr {
// This attribute has no spellings as it is only ever created implicitly.
let Spellings = [];
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 698baf853507f4..bf82e50c8b59e0 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -730,6 +730,8 @@ void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
bool noinline = false;
bool alwaysinline = false;
bool noconvergent = false;
+ HLSLBranchHintAttr::Spelling flattenOrBranch =
+ HLSLBranchHintAttr::SpellingNotCalculated;
const CallExpr *musttail = nullptr;
for (const auto *A : S.getAttrs()) {
@@ -761,6 +763,9 @@ void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
Builder.CreateAssumption(AssumptionVal);
}
} break;
+ case attr::HLSLBranchHint: {
+ flattenOrBranch = cast<HLSLBranchHintAttr>(A)->getSemanticSpelling();
+ } break;
}
}
SaveAndRestore save_nomerge(InNoMergeAttributedStmt, nomerge);
@@ -768,6 +773,8 @@ void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
SaveAndRestore save_alwaysinline(InAlwaysInlineAttributedStmt, alwaysinline);
SaveAndRestore save_noconvergent(InNoConvergentAttributedStmt, noconvergent);
SaveAndRestore save_musttail(MustTailCall, musttail);
+ SaveAndRestore save_flattenOrBranch(HLSLBranchHintAttributedSpelling,
+ flattenOrBranch);
EmitStmt(S.getSubStmt(), S.getAttrs());
}
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 6ead45793742d6..a790d1fde0e104 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2052,6 +2052,7 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
llvm::MDNode *Weights = nullptr;
llvm::MDNode *Unpredictable = nullptr;
+ llvm::MDNode *ControlFlowHint = nullptr;
// If the branch has a condition wrapped by __builtin_unpredictable,
// create metadata that specifies that the branch is unpredictable.
@@ -2076,7 +2077,18 @@ void CodeGenFunction::EmitBranchOnBoolExpr(
Weights = createProfileWeights(TrueCount, CurrentCount - TrueCount);
}
- Builder.CreateCondBr(CondV, TrueBlock, FalseBlock, Weights, Unpredictable);
+ switch (HLSLBranchHintAttributedSpelling) {
+
+ case HLSLBranchHintAttr::Microsoft_branch:
+ case HLSLBranchHintAttr::Microsoft_flatten:
+ ControlFlowHint = createControlFlowHint(HLSLBranchHintAttributedSpelling);
+ break;
+ case HLSLBranchHintAttr::SpellingNotCalculated:
+ break;
+ }
+
+ Builder.CreateCondBr(CondV, TrueBlock, FalseBlock, Weights, Unpredictable,
+ ControlFlowHint);
}
/// ErrorUnsupported - Print out an error that codegen doesn't support the
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index fcc1013d7361ec..d3f1af6f5a5ce9 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -615,6 +615,10 @@ class CodeGenFunction : public CodeGenTypeCache {
/// True if the current statement has noconvergent attribute.
bool InNoConvergentAttributedStmt = false;
+ /// HLSL Branch attribute.
+ HLSLBranchHintAttr::Spelling HLSLBranchHintAttributedSpelling =
+ HLSLBranchHintAttr::SpellingNotCalculated;
+
// The CallExpr within the current statement that the musttail attribute
// applies to. nullptr if there is no 'musttail' on the current statement.
const CallExpr *MustTailCall = nullptr;
@@ -1612,6 +1616,8 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Bitmap used by MC/DC to track condition outcomes of a boolean expression.
Address MCDCCondBitmapAddr = Address::invalid();
+ llvm::MDNode *createControlFlowHint(HLSLBranchHintAttr::Spelling S) const;
+
/// Calculate branch weights appropriate for PGO data
llvm::MDNode *createProfileWeights(uint64_t TrueCount,
uint64_t FalseCount) const;
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 820bb521ccf850..ff3826ca6da854 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1451,6 +1451,19 @@ static uint32_t scaleBranchWeight(uint64_t Weight, uint64_t Scale) {
return Scaled;
}
+llvm::MDNode *
+CodeGenFunction::createControlFlowHint(HLSLBranchHintAttr::Spelling S) const {
+ llvm::MDBuilder MDHelper(CGM.getLLVMContext());
+
+ SmallVector<llvm::Metadata *, 2> Vals(llvm::ArrayRef<llvm::Metadata *>{
+ MDHelper.createString("dx.controlflow.hints"),
+ S == HLSLBranchHintAttr::Spelling::Microsoft_branch
+ ? MDHelper.createConstant(llvm::ConstantInt::get(Int32Ty, 1))
+ : MDHelper.createConstant(llvm::ConstantInt::get(Int32Ty, 2))});
+
+ return llvm::MDNode::get(CGM.getLLVMContext(), Vals);
+}
+
llvm::MDNode *CodeGenFunction::createProfileWeights(uint64_t TrueCount,
uint64_t FalseCount) const {
// Check for empty weights.
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index f801455596fe6f..827a674ece4bc0 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -623,6 +623,12 @@ static Attr *handleHLSLLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
return ::new (S.Context) HLSLLoopHintAttr(S.Context, A, UnrollFactor);
}
+static Attr *handleHLSLBranchHint(Sema &S, Stmt *St, const ParsedAttr &A,
+ SourceRange Range) {
+
+ return ::new (S.Context) HLSLBranchHintAttr(S.Context, A);
+}
+
static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
SourceRange Range) {
if (A.isInvalid() || A.getKind() == ParsedAttr::IgnoredAttribute)
@@ -659,6 +665,8 @@ static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
return handleLoopHintAttr(S, St, A, Range);
case ParsedAttr::AT_HLSLLoopHint:
return handleHLSLLoopHintAttr(S, St, A, Range);
+ case ParsedAttr::AT_HLSLBranchHint:
+ return handleHLSLBranchHint(S, St, A, Range);
case ParsedAttr::AT_OpenCLUnrollHint:
return handleOpenCLUnrollHint(S, St, A, Range);
case ParsedAttr::AT_Suppress:
diff --git a/clang/test/AST/HLSL/HLSLBranchHint.hlsl b/clang/test/AST/HLSL/HLSLBranchHint.hlsl
new file mode 100644
index 00000000000000..907d6c5ff580c0
--- /dev/null
+++ b/clang/test/AST/HLSL/HLSLBranchHint.hlsl
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-compute -ast-dump %s | FileCheck %s
+
+// CHECK: FunctionDecl 0x{{[0-9A-Fa-f]+}} <{{.*}}> {{.*}} used branch 'int (int)'
+// CHECK: AttributedStmt 0x{{[0-9A-Fa-f]+}} <<invalid sloc>
+// CHECK-NEXT: HLSLBranchHintAttr 0x{{[0-9A-Fa-f]+}} <{{.*}}> branch
+export int branch(int X){
+ int resp;
+ [branch] if (X > 0) {
+ resp = -X;
+ } else {
+ resp = X * 2;
+ }
+
+ return resp;
+}
+
+// CHECK: FunctionDecl 0x{{[0-9A-Fa-f]+}} <{{.*}}> {{.*}} used flatten 'int (int)'
+// CHECK: AttributedStmt 0x{{[0-9A-Fa-f]+}} <<invalid sloc>
+// CHECK-NEXT: HLSLBranchHintAttr 0x{{[0-9A-Fa-f]+}} <{{.*}}> flatten
+export int flatten(int X){
+ int resp;
+ [flatten] if (X > 0) {
+ resp = -X;
+ } else {
+ resp = X * 2;
+ }
+
+ return resp;
+}
+
+// CHECK: FunctionDecl 0x{{[0-9A-Fa-f]+}} <{{.*}}> {{.*}} used no_attr 'int (int)'
+// CHECK-NO: AttributedStmt 0x{{[0-9A-Fa-f]+}} <<invalid sloc>
+// CHECK-NO: HLSLBranchHintAttr
+export int no_attr(int X){
+ int resp;
+ if (X > 0) {
+ resp = -X;
+ } else {
+ resp = X * 2;
+ }
+
+ return resp;
+}
diff --git a/clang/test/CodeGenHLSL/HLSLBranchHint.hlsl b/clang/test/CodeGenHLSL/HLSLBranchHint.hlsl
new file mode 100644
index 00000000000000..11b14b1ec43175
--- /dev/null
+++ b/clang/test/CodeGenHLSL/HLSLBranchHint.hlsl
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple dxil-pc-shadermodel6.3-library %s -fnative-half-type -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -finclude-default-header -x hlsl -triple spirv-vulkan-library %s -fnative-half-type -emit-llvm -o - | FileCheck %s
+
+// CHECK: define {{.*}} i32 {{.*}}test_branch{{.*}}(i32 {{.*}} [[VALD:%.*]])
+// CHECK: [[PARAM:%.*]] = load i32, ptr [[VALD]].addr, align 4
+// CHECK: [[CMP:%.*]] = icmp sgt i32 [[PARAM]], 0
+// CHECK: br i1 [[CMP]], label %if.then, label %if.else, !dx.controlflow.hints [[HINT_BRANCH:![0-9]+]]
+export int test_branch(int X){
+ int resp;
+ [branch] if (X > 0) {
+ resp = -X;
+ } else {
+ resp = X * 2;
+ }
+
+ return resp;
+}
+
+// CHECK: define {{.*}} i32 {{.*}}test_flatten{{.*}}(i32 {{.*}} [[VALD:%.*]])
+// CHECK: [[PARAM:%.*]] = load i32, ptr [[VALD]].addr, align 4
+// CHECK: [[CMP:%.*]] = icmp sgt i32 [[PARAM]], 0
+// CHECK: br i1 [[CMP]], label %if.then, label %if.else, !dx.controlflow.hints [[HINT_FLATTEN:![0-9]+]]
+export int test_flatten(int X){
+ int resp;
+ [flatten] if (X > 0) {
+ resp = -X;
+ } else {
+ resp = X * 2;
+ }
+
+ return resp;
+}
+
+// CHECK: define {{.*}} i32 {{.*}}test_no_attr{{.*}}(i32 {{.*}} [[VALD:%.*]])
+// CHECK-NO: !dx.controlflow.hints
+export int test_no_attr(int X){
+ int resp;
+ if (X > 0) {
+ resp = -X;
+ } else {
+ resp = X * 2;
+ }
+
+ return resp;
+}
+
+//CHECK: [[HINT_BRANCH]] = !{!"dx.controlflow.hints", i32 1}
+//CHECK: [[HINT_FLATTEN]] = !{!"dx.controlflow.hints", i32 2}
diff --git a/llvm/include/llvm/IR/FixedMetadataKinds.def b/llvm/include/llvm/IR/FixedMetadataKinds.def
index df572e8791e13b..a239a076d131f7 100644
--- a/llvm/include/llvm/IR/FixedMetadataKinds.def
+++ b/llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -53,3 +53,6 @@ LLVM_FIXED_MD_KIND(MD_DIAssignID, "DIAssignID", 38)
LLVM_FIXED_MD_KIND(MD_coro_outside_frame, "coro.outside.frame", 39)
LLVM_FIXED_MD_KIND(MD_mmra, "mmra", 40)
LLVM_FIXED_MD_KIND(MD_noalias_addrspace, "noalias.addrspace", 41)
+// TODO: this will likelly be placed somewhere else,
+// so we don't mix dxil/hlsl/spirv and clang metadata
+LLVM_FIXED_MD_KIND(MD_dxil_controlflow_hints, "dx.controlflow.hints", 42)
diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h
index 23fd8350a29b3d..215d3362bd4f6c 100644
--- a/llvm/include/llvm/IR/IRBuilder.h
+++ b/llvm/include/llvm/IR/IRBuilder.h
@@ -1101,11 +1101,14 @@ class IRBuilderBase {
/// instruction.
/// \returns The annotated instruction.
template <typename InstTy>
- InstTy *addBranchMetadata(InstTy *I, MDNode *Weights, MDNode *Unpredictable) {
+ InstTy *addBranchMetadata(InstTy *I, MDNode *Weights, MDNode *Unpredictable,
+ MDNode *ControlFlowHint = nullptr) {
if (Weights)
I->setMetadata(LLVMContext::MD_prof, Weights);
if (Unpredictable)
I->setMetadata(LLVMContext::MD_unpredictable, Unpredictable);
+ if (ControlFlowHint)
+ I->setMetadata(LLVMContext::MD_dxil_controlflow_hints, ControlFlowHint);
return I;
}
@@ -1143,9 +1146,11 @@ class IRBuilderBase {
/// instruction.
BranchInst *CreateCondBr(Value *Cond, BasicBlock *True, BasicBlock *False,
MDNode *BranchWeights = nullptr,
- MDNode *Unpredictable = nullptr) {
+ MDNode *Unpredictable = nullptr,
+ MDNode *ControlFlowHint = nullptr) {
return Insert(addBranchMetadata(BranchInst::Create(True, False, Cond),
- BranchWeights, Unpredictable));
+ BranchWeights, Unpredictable,
+ ControlFlowHint));
}
/// Create a conditional 'br Cond, TrueDest, FalseDest'
diff --git a/llvm/include/llvm/IR/ProfDataUtils.h b/llvm/include/llvm/IR/ProfDataUtils.h
index 0bea517df832e3..49f7da480f928d 100644
--- a/llvm/include/llvm/IR/ProfDataUtils.h
+++ b/llvm/include/llvm/IR/ProfDataUtils.h
@@ -48,6 +48,13 @@ bool hasValidBranchWeightMD(const Instruction &I);
/// Nullptr otherwise.
MDNode *getBranchWeightMDNode(const Instruction &I);
+/// Get the branching metadata information
+///
+/// \param I The Instruction to get the weights from.
+/// \returns A pointer to I's branch weights metadata node, if it exists.
+/// Nullptr otherwise.
+MDNode *getDxBranchHint(const Instruction &I);
+
/// Get the valid branch weights metadata node
///
/// \param I The Instruction to get the weights from.
diff --git a/llvm/lib/IR/ProfDataUtils.cpp b/llvm/lib/IR/ProfDataUtils.cpp
index 5441228b3291ee..47a5059017a48f 100644
--- a/llvm/lib/IR/ProfDataUtils.cpp
+++ b/llvm/lib/IR/ProfDataUtils.cpp
@@ -150,6 +150,13 @@ MDNode *getBranchWeightMDNode(const Instruction &I) {
return ProfileData;
}
+MDNode *getDxBranchHint(const Instruction &I) {
+ MDNode *Node = I.getMetadata(LLVMContext::MD_dxil_controlflow_hints);
+ if (!isTargetMD(Node, "dx.controlflow.hints", 2))
+ return nullptr;
+ return Node;
+}
+
MDNode *getValidBranchWeightMDNode(const Instruction &I) {
auto *ProfileData = getBranchWeightMDNode(I);
if (ProfileData && getNumBranchWeights(*ProfileData) == I.getNumSuccessors())
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 8a8835e0269200..5576f46cf89f9a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -2694,12 +2694,8 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
}
return MIB.constrainAllUses(TII, TRI, RBI);
}
- case Intrinsic::spv_loop_merge:
- case Intrinsic::spv_selection_merge: {
- const auto Opcode = IID == Intrinsic::spv_selection_merge
- ? SPIRV::OpSelectionMerge
- : SPIRV::OpLoopMerge;
- auto MIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(Opcode));
+ case Intrinsic::spv_loop_merge: {
+ auto MIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpLoopMerge));
for (unsigned i = 1; i < I.getNumExplicitOperands(); ++i) {
assert(I.getOperand(i).isMBB());
MIB.addMBB(I.getOperand(i).getMBB());
@@ -2707,6 +2703,32 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
MIB.addImm(SPIRV::SelectionControl::None);
return MIB.constrainAllUses(TII, TRI, RBI);
}
+ case Intrinsic::spv_selection_merge: {
+
+ auto SelectionControl = SPIRV::SelectionControl::None;
+ const MDNode *MDOp =
+ I.getOperand(I.getNumExplicitOperands() - 1).getMetadata();
+ if (MDOp->getNumOperands() > 0) {
+ ConstantInt *BranchHint =
+ mdconst::extract<ConstantInt>(MDOp->getOperand(1));
+
+ if (BranchHint->equalsInt(2))
+ SelectionControl = SPIRV::SelectionControl::Flatten;
+ else if (BranchHint->equalsInt(1))
+ SelectionControl = SPIRV::SelectionControl::DontFlatten;
+ else
+ llvm_unreachable("Invalid value for SelectionControl");
+ }
+
+ auto MIB =
+ BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpSelectionMerge));
+ for (unsigned i = 1; i < I.getNumExplicitOperands() - 1; ++i) {
+ assert(I.getOperand(i).isMBB());
+ MIB.addMBB(I.getOperand(i).getMBB());
+ }
+ MIB.addImm(SelectionControl);
+ return MIB.constrainAllUses(TII, TRI, RBI);
+ }
case Intrinsic::spv_cmpxchg:
return selectAtomicCmpXchg(ResVReg, ResType, I);
case Intrinsic::spv_unreachable:
diff --git a/llvm/lib/Target/SPIRV/SPIRVStructurizer.cpp b/llvm/lib/Target/SPIRV/SPIRVStructurizer.cpp
index 13e05b67927518..57285869b2e338 100644
--- a/llvm/lib/Target/SPIRV/SPIRVStructurizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVStructurizer.cpp
@@ -23,6 +23,7 @@
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Intrinsics.h"
#include "llvm/IR/IntrinsicsSPIRV.h"
+#include "llvm/IR/ProfDataUtils.h"
#include "llvm/InitializePasses.h"
#include "llvm/Transforms/Utils/Cloning.h"
#include "llvm/Transforms/Utils/LoopSimplify.h"
@@ -644,8 +645,7 @@ class SPIRVStructurizer : public FunctionPass {
Builder.SetInsertPoint(Header->getTerminator());
auto MergeAddress = BlockAddress::get(BB.getParent(), &BB);
- SmallVector<Value *, 1> Args = {MergeAddress};
- Builder.CreateIntrinsic(Intrinsic::spv_selection_merge, {}, {Args});
+ createOpSelectMerge(&Builder, MergeAddress);
Modified = true;
}
@@ -767,10 +767,9 @@ class SPIRVStructurizer : public FunctionPass {
BasicBlock *Merge = Candidates[0];
auto MergeAddress = BlockAddress::get(Merge->getParent(), Merge);
- SmallVector<Value *, 1> Args = {MergeAddress};
IRBuilder<> Builder(&BB);
Builder.SetInsertPoint(BB.getTerminator());
- Builder.CreateIntrinsic(Intrinsic::spv_selection_merge, {}, {Args});
+ createOpSelectMerge(&Builder, MergeAddress);
}
return Modified;
@@ -1103,8 +1102,7 @@ class SPIRVStructurizer : public FunctionPass {
Builder.SetInsertPoint(Header->getTerminator());
auto MergeAddress = BlockAddress::get(Merge->getParent(), Merge);
- SmallVector<Value *, 1> Args = {MergeAddress};
- Builder.CreateIntrinsic(Intrinsic::spv_selection_merge, {}, {Args});
+ createOpSelectMerge(&Builder, MergeAddress);
continue;
}
@@ -1118,8 +1116,7 @@ class SPIRVStructurizer : public FunctionPass {
Builder.SetInsertPoint(Header->getTerminator());
auto MergeAddress = BlockAddress::get(NewMerge->getParent(), NewMerge);
- SmallVector<Value *, 1> Args = {MergeAddress};
- Builder.CreateIntrinsic(Intrinsic::spv_selection_merge, {}, {Args});
+ createOpSelectMerge(&Builder, MergeAddress);
}
return Modified;
@@ -1206,6 +1203,18 @@ class SPIRVStructurizer : public FunctionPass {
AU.addPreserved<SPIRVConvergenceRegionAnalysisWrapperPass>();
FunctionPass::getAnalysisUsage(AU);
}
+
+ void createOpSelectMerge(IRBuilder<> *Builder, BlockAddress *MergeAddress) {
+ Instruction *BBTerminatoInst = Builder->GetInsertBlock()->getTerminator();
+
+ MDNode *BranchMdNode = getDxBranchHint(*BBTerminatoInst);
+ Value *MDNodeValue =
+ MetadataAsValue::get(Builder->getContext(), BranchMdNode);
+
+ llvm::SmallVector<llvm::Value *, 2> Args = {MergeAddress, MDNodeValue};
+
+ Builder->CreateIntrinsic(Intrinsic::spv_selection_merge, {}, {Args});
+ }
};
} // namespace llvm
diff --git a/llvm/test/CodeGen/DirectX/HLSLBranchHint.ll b/llvm/test/CodeGen/DirectX/HLSLBranchHint.ll
new file mode 100644
index 00000000000000..e7128d19283e80
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/HLSLBranchHint.ll
@@ -0,0 +1,95 @@
+; RUN: opt -S -dxil-op-lower -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+
+; This test make sure LLVM metadata is propagating to DXIL.
+
+
+; CHECK: define i32 @test_branch(i32 %X)
+; CHECK: br i1 %cmp, label %if.then, label %if.else, !dx.controlflow.hints [[HINT_BRANCH:![0-9]+]]
+define i32 @test_branch(i32 %X) {
+entry:
+ %X.addr = alloca i32, align 4
+ %resp = alloca i32, align 4
+ store i32 %X, ptr %X.addr, align 4
+ %0 = load i32, ptr %X.addr, align 4
+ %cmp = icmp sgt i32 %0, 0
+ br i1 %cmp, label %if.then, label %if.else, !dx.controlflow.hints !0
+
+if.then: ; preds = %entry
+ %1 = load i32, ptr %X.addr, align 4
+ %sub = sub nsw i32 0, %1
+ store i32 %sub, ptr %resp, align 4
+ br label %if.end
+
+if.else: ; preds = %entry
+ %2 = load i32, ptr %X.addr, align 4
+ %mul = mul nsw i32 %2, 2
+ store i32 %mul, ptr %resp, align 4
+ br label %if.end
+
+if.end: ; preds = %if.else, %if.then
+ %3 = load i32, ptr %resp, align 4
+ ret i32 %3
+}
+
+
+...
[truncated]
|
There was a problem hiding this comment.
I think we need to think a bit about the IR metadata in the middle of the compiler and how it will be used in various scenarios.
DXIL and SPIR-V both have branch hints that indicate to the GPU backends if a branch should be profitable to flatten or preserve.
- We shouldn't use a DirectX-named metadata field to drive behaviors in the SPIR-V backend.
- In the case where we load SPIR-V or DXIL into LLVM and then lower to a GPU ISA (like AMDGPU), we'll want to translate SPIR-V and DXIL's annotations into a generic LLVM annotation that backends can broadly accept. This same case comes into play if we ever support direct from HLSL -> ISA compiling where we'll want the backends to respect the metadata.
joaosaffran
force-pushed
the
hlsl/ms-if
branch
from
475cce7 to
1ea547b
Compare
There was a problem hiding this comment.
LGTM from me, and thanks for cleaning this up so promptly. Also, for the record, if you want to put things in MD_prof, please do add support to ProfDataUtils. We're happy to support all the various usecases, so if your stuff belongs there, please add it. Also, if there's a greater need for managing various metadata, I'd be happy to discuss how to support things other than MD_prof in a more general support library.
| BrInst->setMetadata("hlsl.controlflow.hint", | ||
| llvm::MDNode::get(CGM.getLLVMContext(), Vals)); | ||
| } break; | ||
| case HLSLControlFlowHintAttr::SpellingNotCalculated: |
There was a problem hiding this comment.
Can this occur? Seems like it probably can't, so maybe this should be a default case with an unreachable?
There was a problem hiding this comment.
SpellingNotCalculated is the default value for HLSLControlFlowAttr. I noticed that this case doesn't make sense to exist, so I will remove it
| auto *HlslControlFlowMD = | ||
| BBTerminatorInst->getMetadata("hlsl.controlflow.hint"); | ||
|
|
||
| if (!HlslControlFlowMD || HlslControlFlowMD->getNumOperands() < 2) |
There was a problem hiding this comment.
Can the operands be < 2 and this still be valid? That seems like something that should be an assert.
There was a problem hiding this comment.
If operands are < 2, the branch has no attributes associated with it, an assert would be triggered every time we have a branch with no attribute.
There was a problem hiding this comment.
If you have a better way to check this default case, please let me know your thoughts, I am not happy with the way this check current works either
There was a problem hiding this comment.
Huh? getMetadata returns null on failure (hence the null check). If it finds the named metadata, it must have two operands right? You even have this asserting on the SPIR-V side.
| case Intrinsic::spv_selection_merge: { | ||
|
|
||
| auto SelectionControl = SPIRV::SelectionControl::None; | ||
| auto LastOp = I.getOperand(I.getNumExplicitOperands() - 1); |
There was a problem hiding this comment.
This code still makes an assumption that if metadata is present on this instruction it must be HLSL branch metadata... That seems like an unsafe assumption to make. Shouldn't we force that we're reading the hlsl.controlflow.hint metadata.
There was a problem hiding this comment.
Stepping back. First off, we don't actually have a test showing the IR post-SPIRVStructurizer, so that part of your change isn't unit tested. The transformation performed in that pass actually might be the root of my concern here.
It looks like you're translating the metadata from the terminator instruction into an argument to a SPIR-V intrinsic, then lowering that intrinsic. That isn't inherently bad, but it makes it a really strange decision to keep the metadata in its language-specific named metadata form as an argument to an intrinsic.
It seems like we should probably translate the metadata into an immediate value passed to the intrinsic rather than keeping it as metadata.
There was a problem hiding this comment.
@Keenuts @s-perron, I am trying to implement this suggestion from @llvm-beanz, but I am facing an issue.
This is the current definition for OpSelectMerge:
def int_spv_loop_merge : Intrinsic<[], [llvm_vararg_ty]>;The suggested solution, requires me to change into this:
def int_spv_selection_merge : Intrinsic<[], [llvm_i32_ty,llvm_vararg_ty], [ImmArg<ArgIndex<0>>]>;The problem I am facing is, the current first parameter of OpSelectMerge is a basic block address, and there is a bunch of places that do similar to the following:
// Otherwise, we need to check: was there an OpSelectionMerge before this
// branch? If we removed the OpBranchConditional, we must also remove the
// OpSelectionMerge. This is not valid for OpLoopMerge:
IntrinsicInst *II =
dyn_cast<IntrinsicInst>(BB->getTerminator()->getPrevNode());
if (!II || II->getIntrinsicID() != Intrinsic::spv_selection_merge)
return;
Constant *C = cast<Constant>(II->getOperand(0));
II->eraseFromParent();
if (!C->isConstantUsed())
C->destroyConstant();
}My question is, should I go through the code and replace everywhere that is reading the first operand to read the second? Or there is a better way to pass this value to op instruct selector?
There was a problem hiding this comment.
def int_spv_loop_merge : Intrinsic<[], [llvm_vararg_ty]>;
Assuming you meant to mention only OpSelectionMerge and not OpLoopMerge.
The suggested solution, requires me to change into this:
def int_spv_selection_merge : Intrinsic<[], [llvm_i32_ty,llvm_vararg_ty], [ImmArg<ArgIndex<0>>]>;
OpSelectionMerge has a fixed number of operands (spec):
This means you should be able to do this:
def int_spv_selection_merge : Intrinsic<[], [llvm_any_ty, llvm_i32_ty]>
Respecting the order of operands of the SPIR-V instruction, hence you shouldn't have to modify the code dealing with it.
So it looks like if you change the intrinsic to have those fixed arguments, you should be able to emit the mask in the frontend when generating the intrinsic call. If you do so, this lowering code will become way simpler.
There was a problem hiding this comment.
@llvm-beanz @Keenuts SPIRV Structurizer pass is not register in opt CLI. Also, I don't see any tests, only calling the structurizer. That being said, there is currently this test, which follow the same pattern as other structurizer tests.
Is such test enough? If any more tests are needed, can you give me an example on how to do it, please?
There was a problem hiding this comment.
Made a separate PR to add the structurizer into opt https://github.com/llvm/llvm-project/pull/119301/files
|
|
||
| MDNode *MDNode = BBTerminatorInst->getMetadata("hlsl.controlflow.hint"); | ||
| if (MDNode) | ||
| assert(MDNode->getNumOperands() == 2 && |
There was a problem hiding this comment.
You shouldn't put an assert as the only statement on an if block, it is better to fold it into the assert condition.
Today most C runtimes are nice about the assert macro defining to a no-op statement because the spec defines it as such, but people do replace assert, and lots of horrible things can happen if a single-statement if's body is a macro expansion and the macro doesn't expand to a complete statement. Things like the next statement becoming the body of the if.
There was a problem hiding this comment.
Thanks, answered the question for the intrinsic.
As Chris said, this would definitely needs to have some tests in the spir-v backend.
Especially given that the structurizer assumes the selection control mask was always None.
When splitting edges, or moving condition, we should make sure the mask is correctly moved/copied, etc
| case Intrinsic::spv_selection_merge: { | ||
|
|
||
| auto SelectionControl = SPIRV::SelectionControl::None; | ||
| auto LastOp = I.getOperand(I.getNumExplicitOperands() - 1); |
There was a problem hiding this comment.
def int_spv_loop_merge : Intrinsic<[], [llvm_vararg_ty]>;
Assuming you meant to mention only OpSelectionMerge and not OpLoopMerge.
The suggested solution, requires me to change into this:
def int_spv_selection_merge : Intrinsic<[], [llvm_i32_ty,llvm_vararg_ty], [ImmArg<ArgIndex<0>>]>;
OpSelectionMerge has a fixed number of operands (spec):
This means you should be able to do this:
def int_spv_selection_merge : Intrinsic<[], [llvm_any_ty, llvm_i32_ty]>
Respecting the order of operands of the SPIR-V instruction, hence you shouldn't have to modify the code dealing with it.
So it looks like if you change the intrinsic to have those fixed arguments, you should be able to emit the mask in the frontend when generating the intrinsic call. If you do so, this lowering code will become way simpler.
joaosaffran
force-pushed
the
hlsl/ms-if
branch
from
9db2081 to
239db34
Compare
| case Intrinsic::spv_selection_merge: { | ||
|
|
||
| int64_t SelectionControl = SPIRV::SelectionControl::None; | ||
| auto LastOp = I.getOperand(I.getNumOperands() - 1); | ||
|
|
||
| auto BranchHint = LastOp.getImm(); | ||
| if (BranchHint == 2) | ||
| SelectionControl = SPIRV::SelectionControl::Flatten; | ||
| else if (BranchHint == 1) | ||
| SelectionControl = SPIRV::SelectionControl::DontFlatten; | ||
|
|
||
| auto MIB = | ||
| BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpSelectionMerge)); | ||
| for (unsigned i = 1; i < I.getNumExplicitOperands() - 1; ++i) { | ||
| assert(I.getOperand(i).isMBB()); | ||
| MIB.addMBB(I.getOperand(i).getMBB()); | ||
| } | ||
| MIB.addImm(SelectionControl); | ||
| return MIB.constrainAllUses(TII, TRI, RBI); | ||
| } |
There was a problem hiding this comment.
Why do you need to loop through the operands given the instruction is not variadic?
(Sadly the Flatten/DontFlatten immediate values don't match the FE values so cannot static cast into the enum :/ )
| case Intrinsic::spv_selection_merge: { | |
| int64_t SelectionControl = SPIRV::SelectionControl::None; | |
| auto LastOp = I.getOperand(I.getNumOperands() - 1); | |
| auto BranchHint = LastOp.getImm(); | |
| if (BranchHint == 2) | |
| SelectionControl = SPIRV::SelectionControl::Flatten; | |
| else if (BranchHint == 1) | |
| SelectionControl = SPIRV::SelectionControl::DontFlatten; | |
| auto MIB = | |
| BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpSelectionMerge)); | |
| for (unsigned i = 1; i < I.getNumExplicitOperands() - 1; ++i) { | |
| assert(I.getOperand(i).isMBB()); | |
| MIB.addMBB(I.getOperand(i).getMBB()); | |
| } | |
| MIB.addImm(SelectionControl); | |
| return MIB.constrainAllUses(TII, TRI, RBI); | |
| } | |
| namespace { | |
| SPIRV::SelectionControl GetSelectionOperandForImm(int Imm) { | |
| if (Imm == 2) | |
| return SPIRV::SelectionControl::Flatten; | |
| if (Imm == 1) | |
| return SPIRV::SelectionControl::DontFlatten; | |
| if (Imm == 0) | |
| return SPIRV::SelectionControl::None; | |
| llvm::unreachable("Invalid immediate") | |
| } | |
| [...] | |
| case Intrinsic::spv_selection_merge: { | |
| auto MIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpSelectionMerge)); | |
| assert(I.getOperand(1).isMBB()); | |
| MIB.addMBB(I.getOperand(1).getMBB()); | |
| MIB.addImm(GetSelectionOperandForImmediate(I.getOperand(2).getImm()); | |
| return MIB.constrainAllUses(TII, TRI, RBI); | |
| } |
| @@ -2081,7 +2081,26 @@ void CodeGenFunction::EmitBranchOnBoolExpr( | |||
| Weights = createProfileWeights(TrueCount, CurrentCount - TrueCount); | |||
| } | |||
|
|
|||
| Builder.CreateCondBr(CondV, TrueBlock, FalseBlock, Weights, Unpredictable); | |||
| auto *BrInst = Builder.CreateCondBr(CondV, TrueBlock, FalseBlock, Weights, | |||
There was a problem hiding this comment.
nit: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
| auto *BrInst = Builder.CreateCondBr(CondV, TrueBlock, FalseBlock, Weights, | |
| llvm::Inst *BrInst = Builder.CreateCondBr(CondV, TrueBlock, FalseBlock, Weights, |
| // CHECK-NO: AttributedStmt 0x{{[0-9A-Fa-f]+}} <<invalid sloc> | ||
| // CHECK-NO: -HLSLControlFlowHintAttr |
There was a problem hiding this comment.
| // CHECK-NO: AttributedStmt 0x{{[0-9A-Fa-f]+}} <<invalid sloc> | |
| // CHECK-NO: -HLSLControlFlowHintAttr | |
| // CHECK-NOT: AttributedStmt 0x{{[0-9A-Fa-f]+}} <<invalid sloc> | |
| // CHECK-NOT: -HLSLControlFlowHintAttr |
This test isn't testing what you think it is... 😄
| } | ||
|
|
||
| // CHECK: define {{.*}} i32 {{.*}}test_no_attr{{.*}}(i32 {{.*}} [[VALD:%.*]]) | ||
| // CHECK-NO: !hlsl.controlflow.hint |
There was a problem hiding this comment.
| // CHECK-NO: !hlsl.controlflow.hint | |
| // CHECK-NOT: !hlsl.controlflow.hint |
| for (auto &F : M) { | ||
| for (auto &BB : F) { | ||
| auto *BBTerminatorInst = BB.getTerminator(); | ||
|
|
||
| auto *HlslControlFlowMD = | ||
| BBTerminatorInst->getMetadata("hlsl.controlflow.hint"); |
There was a problem hiding this comment.
nit: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
| for (auto &F : M) { | |
| for (auto &BB : F) { | |
| auto *BBTerminatorInst = BB.getTerminator(); | |
| auto *HlslControlFlowMD = | |
| BBTerminatorInst->getMetadata("hlsl.controlflow.hint"); | |
| for (Function &F : M) { | |
| for (BasicBlock &BB : F) { | |
| Inst *BBTerminatorInst = BB.getTerminator(); | |
| MDNode *HlslControlFlowMD = | |
| BBTerminatorInst->getMetadata("hlsl.controlflow.hint"); |
| case Intrinsic::spv_selection_merge: { | ||
| auto MIB = | ||
| BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpSelectionMerge)); | ||
| assert(I.getOperand(1).isMBB()); |
There was a problem hiding this comment.
nit: strings added to asserts make it a lot easier to understand and find problems.
| assert(I.getOperand(1).isMBB()); | |
| assert(I.getOperand(1).isMBB() && | |
| "operand 1 to spv_selection_merge must be a basic block"); |
| FPM.add(new SPIRVConvergenceRegionAnalysisWrapperPass()); | ||
| FPM.add(createSPIRVStructurizerPass()); | ||
|
|
||
| if (!FPM.run(F)) |
There was a problem hiding this comment.
This is real hacky... It seems way wrong to have the pass wrapper running a whole bunch of additional passes especially passes that mutate the IR. That defeats the point of being able to run a pass in isolation to test it.
There was a problem hiding this comment.
For the LoopSimplify pass itself, we could probably remove it, and expect the user respect the preconditions.
For the others, aren't all supporting both pass managers?
There was a problem hiding this comment.
I've removed the additional passes, now pass wrapper is running only structurizer pass. Though those were required as they were dependent passes, sorry for the confusion.
joaosaffran
force-pushed
the
hlsl/ms-if
branch
from
4e4ee84 to
e82909b
Compare
joaosaffran
force-pushed
the
hlsl/ms-if
branch
from
e82909b to
b9d0e31
Compare
|
Hi @joaosaffran I believe this patch is causing an build error in |
|
Sorry about that @mustartt, will fix it and send a PR in a few minutes |
|
Thanks! Not a problem, there were another earlier scyl related failure so the bots on other platforms didn't pick this one up. |
This reverts commit 1a435fe.
This reverts commit 0d5c072.
This reverts commit b66f6b2.
- Adding the changes from PRs: - #116331 - #121852 - Fixes test `tools/dxil-dis/debug-info.ll` - Address some missed comments in the previous PR --------- Co-authored-by: joaosaffran <[email protected]>
…122157) - Adding the changes from PRs: - llvm#116331 - llvm#121852 - Fixes test `tools/dxil-dis/debug-info.ll` - Address some missed comments in the previous PR --------- Co-authored-by: joaosaffran <[email protected]>
Closes #70112