[PATCH] D33800: [SelectionDAG] Update the dominator after splitting critical edges
Davide Italiano via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 1 14:32:25 PDT 2017
davide created this revision.
Running `llc -verify-dom-info` on the attached testcase results in a crash in the verifier, due to a stale dominator tree.
i.e.
DominatorTree is not up to date!
Computed:
=============================--------------------------------
Inorder Dominator Tree:
[1] %safe_mod_func_uint8_t_u_u.exit.i.i.i {0,7}
[2] %lor.lhs.false.i61.i.i.i {1,2}
[2] %safe_mod_func_int8_t_s_s.exit.i.i.i {3,6}
[3] %safe_div_func_int64_t_s_s.exit66.i.i.i {4,5}
Actual:
=============================--------------------------------
Inorder Dominator Tree:
[1] %safe_mod_func_uint8_t_u_u.exit.i.i.i {0,9}
[2] %lor.lhs.false.i61.i.i.i {1,2}
[2] %safe_mod_func_int8_t_s_s.exit.i.i.i {3,8}
[3] %safe_div_func_int64_t_s_s.exit66.i.i.i {4,5}
[3] %safe_mod_func_int8_t_s_s.exit.i.i.i.lor.lhs.false.i61.i.i.i_crit_edge {6,7}
This is because in `SelectionDAGIsel` we split critical edges without updating the corresponding dominator for the function (and we claim in `MachineFunctionPass::getAnalysisUsage()` that the domtree is preserved).
We could either stop preserving the domtree in `getAnalysisUsage` or tell `splitCriticalEdge()` to update it.
As the second option is easy to implement, that's the one I chose.
Side note: not sure why this went unnoticed for so long.
https://reviews.llvm.org/D33800
Files:
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
test/CodeGen/X86/O0-pipeline.ll
test/CodeGen/X86/pr33266.ll
Index: test/CodeGen/X86/pr33266.ll
===================================================================
--- /dev/null
+++ test/CodeGen/X86/pr33266.ll
@@ -0,0 +1,29 @@
+; Make sure we don't crash because we have a stale dominator tree.
+; REQUIRES: asserts
+; RUN: llc -verify-dom-info %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+ at global = external global [8 x [8 x [4 x i8]]], align 2
+ at global.1 = external global { i8, [3 x i8] }, align 4
+
+define void @patatino() local_unnamed_addr {
+bb:
+ br label %bb1
+
+bb1:
+ br label %bb2
+
+bb2:
+ br i1 icmp ne (i8* getelementptr inbounds ({ i8, [3 x i8] }, { i8, [3 x i8] }* @global.1, i64 0, i32 0), i8* getelementptr inbounds ([8 x [8 x [4 x i8]]], [8 x [8 x [4 x i8]]]* @global, i64 0, i64 6, i64 6, i64 2)), label %bb4, label %bb3
+
+bb3:
+ br i1 icmp eq (i64 ashr (i64 shl (i64 zext (i32 srem (i32 7, i32 zext (i1 icmp eq (i8* getelementptr inbounds ({ i8, [3 x i8] }, { i8, [3 x i8] }* @global.1, i64 0, i32 0), i8* getelementptr inbounds ([8 x [8 x [4 x i8]]], [8 x [8 x [4 x i8]]]* @global, i64 0, i64 6, i64 6, i64 2)) to i32)) to i64), i64 56), i64 56), i64 0), label %bb5, label %bb4
+
+bb4:
+ %tmp = phi i64 [ ashr (i64 shl (i64 zext (i32 srem (i32 7, i32 zext (i1 icmp eq (i8* getelementptr inbounds ({ i8, [3 x i8] }, { i8, [3 x i8] }* @global.1, i64 0, i32 0), i8* getelementptr inbounds ([8 x [8 x [4 x i8]]], [8 x [8 x [4 x i8]]]* @global, i64 0, i64 6, i64 6, i64 2)) to i32)) to i64), i64 56), i64 56), %bb3 ], [ 7, %bb2 ]
+ ret void
+
+bb5:
+ ret void
+}
Index: test/CodeGen/X86/O0-pipeline.ll
===================================================================
--- test/CodeGen/X86/O0-pipeline.ll
+++ test/CodeGen/X86/O0-pipeline.ll
@@ -32,6 +32,7 @@
; CHECK-NEXT: Safe Stack instrumentation pass
; CHECK-NEXT: Insert stack protectors
; CHECK-NEXT: Module Verifier
+; CHECK-NEXT: Dominator Tree Construction
; CHECK-NEXT: X86 DAG->DAG Instruction Selection
; CHECK-NEXT: X86 PIC Global Base Reg Initialization
; CHECK-NEXT: Expand ISel Pseudo-instructions
Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -328,6 +328,9 @@
if (UseMBPI && OptLevel != CodeGenOpt::None)
AU.addRequired<BranchProbabilityInfoWrapperPass>();
MachineFunctionPass::getAnalysisUsage(AU);
+
+ // The dominator tree is preserved in MachineFunctionPass::getAnalysisUsage()
+ AU.addRequired<DominatorTreeWrapperPass>();
}
/// SplitCriticalSideEffectEdges - Look for critical edges with a PHI value that
@@ -337,7 +340,7 @@
///
/// This is required for correctness, so it must be done at -O0.
///
-static void SplitCriticalSideEffectEdges(Function &Fn) {
+static void SplitCriticalSideEffectEdges(Function &Fn, DominatorTree *DT) {
// Loop for blocks with phi nodes.
for (BasicBlock &BB : Fn) {
PHINode *PN = dyn_cast<PHINode>(BB.begin());
@@ -363,7 +366,7 @@
// Okay, we have to split this edge.
SplitCriticalEdge(
Pred->getTerminator(), GetSuccessorNumber(Pred, &BB),
- CriticalEdgeSplittingOptions().setMergeIdenticalEdges());
+ CriticalEdgeSplittingOptions(DT).setMergeIdenticalEdges());
goto ReprocessBlock;
}
}
@@ -399,10 +402,11 @@
LibInfo = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
GFI = Fn.hasGC() ? &getAnalysis<GCModuleInfo>().getFunctionInfo(Fn) : nullptr;
ORE = make_unique<OptimizationRemarkEmitter>(&Fn);
+ DominatorTree *DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
DEBUG(dbgs() << "\n\n\n=== " << Fn.getName() << "\n");
- SplitCriticalSideEffectEdges(const_cast<Function &>(Fn));
+ SplitCriticalSideEffectEdges(const_cast<Function &>(Fn), DT);
CurDAG->init(*MF, *ORE);
FuncInfo->set(Fn, *MF, CurDAG);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D33800.101109.patch
Type: text/x-patch
Size: 3951 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170601/783365d5/attachment.bin>
More information about the llvm-commits
mailing list