<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hey Marcello<div><br></div><div>Very minor nitpick, but can you fix the formatting of this code</div><div><br></div><div><div>+ DEBUG(dbgs() << "LHS Bounds ==> ";</div><div>+ if (LowerBound) {</div><div>+ dbgs() << cast<ConstantInt>(LowerBound)->getSExtValue();</div><div>+ } else { dbgs() << "NONE"; } dbgs()</div><div>+ << " - " << NewUpperBound->getSExtValue() << "\n";</div><div>+ dbgs() << "RHS Bounds ==> ";</div><div>+ dbgs() << NewLowerBound->getSExtValue() << " - "; if (UpperBound) {</div><div>+ dbgs() << cast<ConstantInt>(UpperBound)->getSExtValue() << "\n";</div><div>+ } else { dbgs() << "NONE\n"; });</div><div><br></div><div>Otherwise LGTM, pending David ok’ing your test updates.</div><div><br></div><div>Thanks,</div><div>Pete</div><div><div>On Jun 10, 2014, at 9:15 AM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com">hayarms@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Here is a patch with the original test 'feature.ll' modified to pass on the new implementation<div><br></div><div>Marcello</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-10 17:06 GMT+01:00 Marcello Maggioni <span dir="ltr"><<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The new test is basically the old test updated to pass on the optimized algorithm.<div><br></div><div>I'll produce a patch with the new test renamed as the old test.</div>
<div><br></div><div>Marcello</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-10 16:19 GMT+01:00 David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span>:<div><div class="h5">
<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On Tue, Jun 10, 2014 at 5:26 AM, Marcello Maggioni <<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>> wrote:<br>
> Hello,<br>
><br>
> when LowerSwitch transforms a switch instruction into a tree of ifs it is<br>
> actually performing a binary search into the various case ranges, to see if<br>
> the current value falls into one cases range of values.<br>
><br>
> So, if we have a program with something like this:<br>
><br>
> switch (a) {<br>
> case 0:<br>
> do0();<br>
> break;<br>
> case 1:<br>
> do1();<br>
> break;<br>
> case 2:<br>
> do2();<br>
> break;<br>
> default:<br>
> break;<br>
> }<br>
><br>
> the code produced is something like this:<br>
><br>
> if (a < 1) {<br>
> if (a == 0) {<br>
> do0();<br>
> }<br>
> } else {<br>
> if (a < 2) {<br>
> if (a == 1) {<br>
> do1();<br>
> }<br>
> } else {<br>
> if (a == 2) {<br>
> do2();<br>
> }<br>
> }<br>
> }<br>
><br>
> This code is inefficient because the check (a == 1) to execute do1() is not<br>
> needed.<br>
><br>
> The reason is that because we already checked that (a >= 1) initially by<br>
> checking that also (a < 2) we basically already inferred that (a == 1)<br>
> without the need of an extra basic block spawned to check if actually (a ==<br>
> 1).<br>
><br>
> The proposed patch addresses this problem by keeping track of already<br>
> checked bounds in the LowerSwitch algorithm, so that when the time arrives<br>
> to produce a Leaf Block that checks the equality with the case value / range<br>
> the algorithm can decide if that block is really needed depending on the<br>
> already checked bounds .<br>
><br>
> For example, the above with "a = 1" would work like this:<br>
><br>
> the bounds start as LB: NONE , UB: NONE<br>
> as (a < 1) is emitted the bounds for the else path become LB: 1 UB: NONE.<br>
> This happens because by failing the test (a < 1) we know that the value "a"<br>
> cannot be smaller than 1 if we enter the else branch.<br>
> After the emitting the check (a < 2) the bounds in the if branch become LB:<br>
> 1 UB: 1. This is because by checking that "a" is smaller than 2 then the<br>
> upper bound becomes 2 - 1 = 1.<br>
><br>
> When is time to emit the leaf block for "case 1:" we notice that 1 is<br>
> squeezed exactly in between the LB and UB, which means that if we arrived to<br>
> that block there is no need to emit a block that checks if (a == 1).<br>
><br>
><br>
> Please let me know if I did something wrong here and if the reasoning is<br>
> correct :)<br>
><br>
> The proposed patch also sets as XFAIL the test:<br>
> "test/Transforms/LowerSwitch/feature.ll".<br>
> This test checks that the emitted lowered CFG is exactly the one expected by<br>
> the previous algorithm without this optimization. Of course this test starts<br>
> failing when this optimization is added. I added another test that is the<br>
> same, but is updated with the new expected CFG.<br>
><br>
> Don't know what is the best thing to do with the original "feature.ll" test<br>
> in the case this patch gets accepted. Should we remove it or keep it as a<br>
> reference?<br>
<br>
</div>Most likely we should just change the original to test the new<br>
behavior, and not introduce a new test. If the original test doesn't<br>
test all cases of the new behavior and can't be reasonably made to do<br>
so, we could introduce a new test for those new cases (while keeping<br>
the old one covering the existing cases, but updated to verify the new<br>
optimization you've implemented is functioning)<br>
<br>
- David<br>
<br>
><br>
> Cheers,<br>
> Marcello<br>
><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></div></div><br></div>
</blockquote></div><br></div>
<span><optimized_lower_switch_v2.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></div></body></html>