<div dir="ltr">Thanks for this!<div><br></div><div>I've committed this with an added flag and more tests.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 14, 2015 at 8:36 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi chandlerc,<br>
<br>
Some benchmarks have shown that this could lead to a potential performance benefit.<br>
<br>
A possible explanation. In diamond-shaped CFGs (A followed by either B or C both followed by D), putting B and C both in between A and D leads to the code being less dense than it could be. Always either B or C have to be skipped increasing the chance of cache misses etc. Moving either B or C to after D might be beneficial on average.<br>
<br>
In the long run, but we should probably do a better job of analyzing the basic block and branch probabilities to move the correct one of B or C to after D. But even if we don't use this in the long run, it is a good baseline for benchmarking.<br>
<br>
<a href="http://reviews.llvm.org/D6969" target="_blank">http://reviews.llvm.org/D6969</a><br>
<br>
Files:<br>
lib/CodeGen/MachineBlockPlacement.cpp<br>
test/CodeGen/X86/code_placement_bad_cfg_check.ll<br>
<br>
Index: lib/CodeGen/MachineBlockPlacement.cpp<br>
===================================================================<br>
--- lib/CodeGen/MachineBlockPlacement.cpp<br>
+++ lib/CodeGen/MachineBlockPlacement.cpp<br>
@@ -60,6 +60,12 @@<br>
"blocks in the function."),<br>
cl::init(0), cl::Hidden);<br>
<br>
+static cl::opt<bool> NoBadCFGConflictCheck(<br>
+ "no-bad-cfg-conflict-check",<br>
+ cl::desc("Don't check whether a hot successor has a more important "<br>
+ "predecessor."),<br>
+ cl::init(false), cl::Hidden);<br>
+<br>
// FIXME: Find a good default for this flag and remove the flag.<br>
static cl::opt<unsigned><br>
ExitBlockBias("block-placement-exit-block-bias",<br>
@@ -374,29 +380,31 @@<br>
continue;<br>
}<br>
<br>
- // Make sure that a hot successor doesn't have a globally more important<br>
- // predecessor.<br>
- BlockFrequency CandidateEdgeFreq<br>
- = MBFI->getBlockFreq(BB) * SuccProb * HotProb.getCompl();<br>
- bool BadCFGConflict = false;<br>
- for (MachineBasicBlock::pred_iterator PI = (*SI)->pred_begin(),<br>
- PE = (*SI)->pred_end();<br>
- PI != PE; ++PI) {<br>
- if (*PI == *SI || (BlockFilter && !BlockFilter->count(*PI)) ||<br>
- BlockToChain[*PI] == &Chain)<br>
+ if (!NoBadCFGConflictCheck) {<br>
+ // Make sure that a hot successor doesn't have a globally more important<br>
+ // predecessor.<br>
+ BlockFrequency CandidateEdgeFreq =<br>
+ MBFI->getBlockFreq(BB) * SuccProb * HotProb.getCompl();<br>
+ bool BadCFGConflict = false;<br>
+ for (MachineBasicBlock::pred_iterator PI = (*SI)->pred_begin(),<br>
+ PE = (*SI)->pred_end();<br>
+ PI != PE; ++PI) {<br>
+ if (*PI == *SI || (BlockFilter && !BlockFilter->count(*PI)) ||<br>
+ BlockToChain[*PI] == &Chain)<br>
+ continue;<br>
+ BlockFrequency PredEdgeFreq =<br>
+ MBFI->getBlockFreq(*PI) * MBPI->getEdgeProbability(*PI, *SI);<br>
+ if (PredEdgeFreq >= CandidateEdgeFreq) {<br>
+ BadCFGConflict = true;<br>
+ break;<br>
+ }<br>
+ }<br>
+ if (BadCFGConflict) {<br>
+ DEBUG(dbgs() << " " << getBlockName(*SI) << " -> " << SuccProb<br>
+ << " (prob) (non-cold CFG conflict)\n");<br>
continue;<br>
- BlockFrequency PredEdgeFreq<br>
- = MBFI->getBlockFreq(*PI) * MBPI->getEdgeProbability(*PI, *SI);<br>
- if (PredEdgeFreq >= CandidateEdgeFreq) {<br>
- BadCFGConflict = true;<br>
- break;<br>
}<br>
}<br>
- if (BadCFGConflict) {<br>
- DEBUG(dbgs() << " " << getBlockName(*SI) << " -> " << SuccProb<br>
- << " (prob) (non-cold CFG conflict)\n");<br>
- continue;<br>
- }<br>
}<br>
<br>
DEBUG(dbgs() << " " << getBlockName(*SI) << " -> " << SuccProb<br>
Index: test/CodeGen/X86/code_placement_bad_cfg_check.ll<br>
===================================================================<br>
--- /dev/null<br>
+++ test/CodeGen/X86/code_placement_bad_cfg_check.ll<br>
@@ -0,0 +1,25 @@<br>
+; RUN: llc -mcpu=corei7 -mtriple=x86_64-linux -no-bad-cfg-conflict-check < %s | FileCheck %s<br>
+<br>
+;CHECK: callq h<br>
+;CHECK: ret<br>
+;CHECK: callq<br>
+define void @foo(i32 %t) nounwind readnone ssp uwtable {<br>
+ %cmp = icmp eq i32 %t, 0<br>
+ br i1 %cmp, label %if.then, label %if.else<br>
+<br>
+if.then:<br>
+ call void @f()<br>
+ br label %if.end<br>
+<br>
+if.else:<br>
+ call void @g()<br>
+ br label %if.end<br>
+<br>
+if.end:<br>
+ call void @h()<br>
+ ret void<br>
+}<br>
+<br>
+declare void @f()<br>
+declare void @g()<br>
+declare void @h()<br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>