<div dir="ltr"><div>Thank you for the explanation! I'll abandon my patch on Phab.<br><br></div>My only suggestion would be to trim the test case if possible.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 20, 2014 at 10:03 AM, Michael Zolotukhin <span dir="ltr"><<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Sanjay,<div><br></div><div>Indeed, we’ve been working on very similar problems, and actually your fix is included into my patch. However, along with the issue your fix is targeting, there is one more. It shows up when trip-count isn’t known at compile time (replace <span style="font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)">%cmp = icmp eq i32 %inc, 0 </span>with <span style="font-family:Menlo,Consolas,Monaco,monospace;font-size:10px;line-height:16px;white-space:pre-wrap;background-color:rgb(170,255,170)">%cmp = icmp eq i32 %inc, %N</span> in your example) and we use unroll-factor=2. Using unroll-factor=2 is crucial to expose this bug, because for other cases the prologue is a loop, and we use this loop if at runtime we find out that the trip count overflows. To make this possible for unroll-factor=2 as well we need to stop unrolling the prologue if overflow is possible.</div><div><br></div><div>I included a test-case to my patch, and return values of @foo differs depending on whether the loop was unrolled or not when trunk compiler is used. You can inspect that by adding a @main function to print the return value.</div><div><br></div><div>Below you can find outputs with and without my change, along with my annotation:</div><div><br></div><div><div>Without my change (please not that it’s actually not a loop):</div><div><font style="font-size:11px" face="Menlo"><div>entry:</div><div> %0 = add i32 %N, 1 ; <<< If %N is -1, %0 overflows and becomes 0 here.</div><div> %xtraiter = and i32 %0, 1</div><div> %lcmp.mod = icmp ne i32 %xtraiter, 0</div><div> %lcmp.overflow = icmp eq i32 %0, 0 ; <<< Here we actually detect the overflow, and our</div><div> ; plan is to go to prologue loop to handle that.</div><div> %lcmp.or = or i1 %lcmp.overflow, %lcmp.mod</div><div> br i1 %lcmp.or, label %while.body.prol, label %entry.split ; <<< Here we jump to the prologue</div><div><br></div></font></div><div><font style="font-size:11px" face="Menlo">while.body.prol: ; preds = %entry</font></div><div><font style="font-size:11px" face="Menlo"> %1 = bitcast i32 0 to float</font></div><div><font style="font-size:11px" face="Menlo"> %cmp.prol = fcmp oge float %1, 0.000000e+00</font></div><div><font style="font-size:11px" face="Menlo"> %cmp2.prol = fcmp ole float %1, 1.000000e+00</font></div><div><font style="font-size:11px" face="Menlo"> %or.cond.prol = and i1 %cmp.prol, %cmp2.prol</font></div><div><font style="font-size:11px" face="Menlo"> %inc.prol = zext i1 %or.cond.prol to i32</font></div><div><font style="font-size:11px" face="Menlo"> %inc.count.0.prol = add i32 0, %inc.prol</font></div><div><font style="font-size:11px" face="Menlo"> %cmp4.prol = icmp eq i32 0, %N ; <<< But it’s not a loop any more!</font></div><div><font style="font-size:11px" face="Menlo"> %inc7.prol = add i32 0, 1 ; <<< We execute a single iteration and proceed</font></div><div><font style="font-size:11px" face="Menlo"> br label %entry.split ; <<<</font></div></div><div><font style="font-size:11px" face="Menlo"><br></font></div><div><font style="font-size:11px" face="Menlo"><div>entry.split: ; preds = %while.body.prol, %entry</div><div> %inc.count.0.lcssa.unr = phi i32 [ 0, %entry ], [ %inc.count.0.prol, %while.body.prol ]</div><div> %storemerge.unr = phi i32 [ 0, %entry ], [ %inc7.prol, %while.body.prol ]</div><div> %count.0.unr = phi i32 [ 0, %entry ], [ %inc.count.0.prol, %while.body.prol ]</div><div> %2 = icmp ult i32 %0, 2</div><div> br i1 %2, label %while.end, label %entry.split.split ; <<< And here we skip the unrolled loop and go to</div><div> ; function exit</div><div><br></div><div>entry.split.split: ; preds = %entry.split</div><div> br label %while.body</div><div><br></div><div>while.body: ; preds = %while.body, %entry.split.split</div><div> %storemerge = phi i32 [ %storemerge.unr, %entry.split.split ], [ %inc7.1, %while.body ]</div><div> %count.0 = phi i32 [ %count.0.unr, %entry.split.split ], [ %inc.count.0.1, %while.body ]</div><div> %3 = bitcast i32 %storemerge to float</div><div> %cmp = fcmp oge float %3, 0.000000e+00</div><div> %cmp2 = fcmp ole float %3, 1.000000e+00</div><div> %or.cond = and i1 %cmp, %cmp2</div><div> %inc = zext i1 %or.cond to i32</div><div> %inc.count.0 = add i32 %count.0, %inc</div><div> %inc7 = add i32 %storemerge, 1</div><div> %4 = bitcast i32 %inc7 to float</div><div> %cmp.1 = fcmp oge float %4, 0.000000e+00</div><div> %cmp2.1 = fcmp ole float %4, 1.000000e+00</div><div> %or.cond.1 = and i1 %cmp.1, %cmp2.1</div><div> %inc.1 = zext i1 %or.cond.1 to i32</div><div> %inc.count.0.1 = add i32 %inc.count.0, %inc.1</div><div> %cmp4.1 = icmp eq i32 %inc7, %N</div><div> %inc7.1 = add i32 %inc7, 1</div><div> br i1 %cmp4.1, label %while.end.unr-lcssa, label %while.body</div></font></div><div><br></div><div><br></div><div>With my change (the difference is only in <span style="font-family:Menlo">while.body.prol</span> - it’s a loop now):</div><div><div><font face="Menlo"><div><br></div><div><span style="font-size:11px">while.body.prol: ; preds = %while.body.prol, %entry</span></div></font></div><div><font style="font-size:11px" face="Menlo"> %storemerge.prol = phi i32 [ 0, %entry ], [ %inc7.prol, %while.body.prol ]</font></div><div><font style="font-size:11px" face="Menlo"> %count.0.prol = phi i32 [ 0, %entry ], [ %inc.count.0.prol, %while.body.prol ]</font></div><div><font style="font-size:11px" face="Menlo"> %prol.iter = phi i32 [ %xtraiter, %entry ], [ %prol.iter.sub, %while.body.prol ]</font></div><div><font style="font-size:11px" face="Menlo"> %1 = bitcast i32 %storemerge.prol to float</font></div><div><font style="font-size:11px" face="Menlo"> %cmp.prol = fcmp oge float %1, 0.000000e+00</font></div><div><font style="font-size:11px" face="Menlo"> %cmp2.prol = fcmp ole float %1, 1.000000e+00</font></div><div><font style="font-size:11px" face="Menlo"> %or.cond.prol = and i1 %cmp.prol, %cmp2.prol</font></div><div><font style="font-size:11px" face="Menlo"> %inc.prol = zext i1 %or.cond.prol to i32</font></div><div><font style="font-size:11px" face="Menlo"> %inc.count.0.prol = add i32 %count.0.prol, %inc.prol</font></div><div><font style="font-size:11px" face="Menlo"> %cmp4.prol = icmp eq i32 %storemerge.prol, %N</font></div><div><font style="font-size:11px" face="Menlo"> %inc7.prol = add i32 %storemerge.prol, 1</font></div><div><font style="font-size:11px" face="Menlo"> %prol.iter.sub = sub i32 %prol.iter, 1</font></div><div><font style="font-size:11px" face="Menlo"> %prol.iter.cmp = icmp ne i32 %prol.iter.sub, 0</font></div><div><font style="font-size:11px" face="Menlo"> br i1 %prol.iter.cmp, label %while.body.prol, label %entry.split, !llvm.loop !0</font></div></div><div><font face="Menlo"><div><br></div></font></div><div>Thanks,</div><div>Michael</div><div><div class="h5"><div><br></div><div><br></div><div><div><blockquote type="cite"><div>On Nov 20, 2014, at 8:19 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br><div><div dir="ltr"><div>Hi Michael,<br><br>I have a similar patch to fix this here:<br><a href="http://reviews.llvm.org/D6200" target="_blank">http://reviews.llvm.org/D6200</a><br><br>Based on this newer bug report:<br><a href="http://llvm.org/bugs/show_bug.cgi?id=21409" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=21409</a><br><br></div>How does your change to the prologue check affect the transform? Can you post example code with and without that change? I minimized the test case in my patch, so it might be easier to explain using that as the example code.<br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 19, 2014 at 6:36 PM, Michael Zolotukhin <span dir="ltr"><<a href="mailto:mzolotukhin@apple.com" target="_blank">mzolotukhin@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi,<div><br></div><div>This patch fixes an issue with possible integer overflow that might be caused by LoopUnroll. The problem was reported here: [1], and than it was partially solved (see [2]), but not completely.</div><div><br></div><div>This fix aims the following issue in LoopUnroll pass: currently LoopUnroll generates a prologue loop before the main loop body to execute first N%UnrollFactor iterations. When UnrollFactor is 2, then this loop performs a single iteration, and thus its back-edge is optimized away, and the loop actually becomes just a linear code. </div><div><br></div><div>That would work fine, but it turns out that we also want to use this un-unrolled loop as a last resort if for some reason we can’t use unrolled loop. One of such reasons is possible trip-count overflow.</div><div><br></div><div>The attached patch makes the condition when we fully unroll the prologue more strict, and thusly fix the issue in question.</div><div><br></div><div>Does it look ok?</div><div><br></div><div></div></div><br><div style="word-wrap:break-word"><div></div><div><br></div><div>Thanks,</div><div>Michael</div><div><br></div><div>[1]: <a href="http://stackoverflow.com/questions/23838661/why-is-clang-optimizing-this-code-out" target="_blank">http://stackoverflow.com/questions/23838661/why-is-clang-optimizing-this-code-out</a></div><div>[2]: <a href="http://llvm.org/bugs/show_bug.cgi?id=19846" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=19846</a></div><div><br></div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">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>
</div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>