<div dir="ltr">Errr, rather than limit recursion depth, why not just keep track of what's visited and avoid the cycles?<div><br></div><div>(You could also cheaply build sccs of the instruction and it's uses, and decide what to do for each SCC as a whole, which would avoid this issue as well)</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 27, 2016 at 10:10 AM, Sanjay Patel via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">spatel created this revision.<br>
spatel added reviewers: majnemer, jmolloy.<br>
spatel added a subscriber: llvm-commits.<br>
Herald added a subscriber: mcrosier.<br>
<br>
This is a fix for:<br>
<a href="https://llvm.org/bugs/show_bug.cgi?id=26308" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=26308</a><br>
<br>
With the switch to using the TTI cost model in:<br>
<a href="http://reviews.llvm.org/rL228826" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL228826</a><br>
...it became possible to hit a zero-cost cycle of instructions (gep -> phi -> gep...), so we need a cap for the recursion in DominatesMergePoint().<br>
<br>
A recursion depth parameter was already added for a different reason in:<br>
<a href="http://reviews.llvm.org/rL255660" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL255660</a><br>
...so I think we just need to set a limit.<br>
<br>
I pulled "10" out of the air and made it an independent parameter that we can play with. It might be higher than it needs to be given the currently low default value of PHINodeFoldingThreshold (2). That's the starting cost value that we enter the recursion with, and most instructions have cost set to TCC_Basic (1), so I don't think we're going to speculate more than 2 instructions with the current parameters.<br>
<br>
<a href="http://reviews.llvm.org/D16637" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16637</a><br>
<br>
Files:<br>
lib/Transforms/Utils/SimplifyCFG.cpp<br>
test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll<br>
<br>
Index: test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll<br>
===================================================================<br>
--- test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll<br>
+++ test/Transforms/SimplifyCFG/X86/switch_to_lookup_table.ll<br>
@@ -1302,3 +1302,35 @@<br>
; CHECK: entry<br>
; CHECK-NEXT: switch<br>
}<br>
+<br>
+; Speculation depth must be limited to avoid a zero-cost instruction cycle.<br>
+<br>
+; CHECK-LABEL: @PR26308(<br>
+; CHECK: cleanup4:<br>
+; CHECK-NEXT: br label %cleanup4<br>
+<br>
+define i32 @PR26308(i1 %B, i64 %load) {<br>
+entry:<br>
+ br label %while.body<br>
+<br>
+while.body:<br>
+ br label %cleanup<br>
+<br>
+cleanup:<br>
+ %cleanup.dest.slot.0 = phi i1 [ false, %while.body ]<br>
+ br i1 %cleanup.dest.slot.0, label %for.cond, label %cleanup4<br>
+<br>
+for.cond:<br>
+ %e.0 = phi i64* [ undef, %cleanup ], [ %incdec.ptr, %for.cond2 ]<br>
+ %pi = ptrtoint i64* %e.0 to i64<br>
+ %incdec.ptr = getelementptr inbounds i64, i64* %e.0, i64 1<br>
+ br label %for.cond2<br>
+<br>
+for.cond2:<br>
+ %storemerge = phi i64 [ %pi, %for.cond ], [ %load, %for.cond2 ]<br>
+ br i1 %B, label %for.cond2, label %for.cond<br>
+<br>
+cleanup4:<br>
+ br label %while.body<br>
+}<br>
+<br>
Index: lib/Transforms/Utils/SimplifyCFG.cpp<br>
===================================================================<br>
--- lib/Transforms/Utils/SimplifyCFG.cpp<br>
+++ lib/Transforms/Utils/SimplifyCFG.cpp<br>
@@ -90,6 +90,11 @@<br>
cl::desc("Allow exactly one expensive instruction to be speculatively "<br>
"executed"));<br>
<br>
+static cl::opt<unsigned> MaxSpeculationDepth(<br>
+ "max-speculation-depth", cl::Hidden, cl::init(10),<br>
+ cl::desc("Limit maximum recursion depth when calculating costs of "<br>
+ "speculatively executed instructions"));<br>
+<br>
STATISTIC(NumBitMaps, "Number of switch instructions turned into bitmaps");<br>
STATISTIC(NumLinearMaps, "Number of switch instructions turned into linear mapping");<br>
STATISTIC(NumLookupTables, "Number of switch instructions turned into lookup tables");<br>
@@ -269,6 +274,11 @@<br>
unsigned &CostRemaining,<br>
const TargetTransformInfo &TTI,<br>
unsigned Depth = 0) {<br>
+ // It is possible to hit a zero-cost cycle (phi/gep instructions for example),<br>
+ // so limit the recursion depth.<br>
+ if (Depth == MaxSpeculationDepth)<br>
+ return false;<br>
+<br>
Instruction *I = dyn_cast<Instruction>(V);<br>
if (!I) {<br>
// Non-instructions all dominate instructions, but not all constantexprs<br>
<br>
<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>