<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Functionality wise, this seems reasonable. There are a few excess
white space changes which should be removed before the patch is
submitted and some of the new code is oddly formatted. Once you
remove the excess diffs, consider running clang-format.<br>
<br>
This is not a LGTM - I don't have commit rights and don't know this
code well enough even if I did. <br>
<br>
Philip<br>
<br>
<br>
<div class="moz-cite-prefix">On 06/10/2014 05:26 AM, Marcello
Maggioni wrote:<br>
</div>
<blockquote
cite="mid:CA+5XV7b9=53DjGp=mNV20Xu1bRxZ4x0wiL1DxrH_eKTRG3Dndw@mail.gmail.com"
type="cite">
<div dir="ltr">Hello,
<div><br>
</div>
<div>when LowerSwitch transforms a switch instruction into a
tree of ifs it is actually performing a binary search into the
various case ranges, to see if the current value falls into
one cases range of values.</div>
<div><br>
</div>
<div>So, if we have a program with something like this:</div>
<div><br>
</div>
<div>switch (a) {</div>
<div>case 0:</div>
<div> do0();</div>
<div> break;</div>
<div>case 1:</div>
<div> do1();</div>
<div> break;</div>
<div>case 2:</div>
<div> do2();</div>
<div> break;</div>
<div>default:</div>
<div> break;</div>
<div>}</div>
<div><br>
</div>
<div>the code produced is something like this:</div>
<div><br>
</div>
<div> if (a < 1) {</div>
<div>
if (a == 0) {</div>
<div> do0();</div>
<div> }</div>
<div> } else {</div>
<div> if (a < 2) {</div>
<div> if (a == 1) {</div>
<div> do1();</div>
<div> }</div>
<div> } else {</div>
<div> if (a == 2) {</div>
<div> do2();</div>
<div> }</div>
<div> }</div>
<div> }</div>
<div><br>
</div>
<div>This code is inefficient because the check (a == 1) to
execute do1() is not needed.</div>
<div><br>
</div>
<div>The reason is that because we already checked that (a >=
1) initially by checking that also (a < 2) we basically
already inferred that (a == 1) without the need of an extra
basic block spawned to check if actually (a == 1).</div>
<div><br>
</div>
<div>The proposed patch addresses this problem by keeping track
of already checked bounds in the LowerSwitch algorithm, so
that when the time arrives to produce a Leaf Block that checks
the equality with the case value / range the algorithm can
decide if that block is really needed depending on the already
checked bounds .</div>
<div><br>
</div>
<div>For example, the above with "a = 1" would work like this:</div>
<div><br>
</div>
<div>the bounds start as LB: NONE , UB: NONE</div>
<div>as (a < 1) is emitted the bounds for the else path
become LB: 1 UB: NONE. This happens because by failing the
test (a < 1) we know that the value "a" cannot be smaller
than 1 if we enter the else branch.</div>
<div>After the emitting the check (a < 2) the bounds in the
if branch become LB: 1 UB: 1. This is because by checking that
"a" is smaller than 2 then the upper bound becomes 2 - 1 = 1.</div>
<div><br>
</div>
<div>
When is time to emit the leaf block for "case 1:" we notice
that 1 is squeezed exactly in between the LB and UB, which
means that if we arrived to that block there is no need to
emit a block that checks if (a == 1).</div>
<div><br>
</div>
<div><br>
</div>
<div>Please let me know if I did something wrong here and if the
reasoning is correct :)</div>
<div><br>
</div>
<div>The proposed patch also sets as XFAIL the test:
"test/Transforms/LowerSwitch/feature.ll".</div>
<div>This test checks that the emitted lowered CFG is exactly
the one expected by the previous algorithm without this
optimization. Of course this test starts failing when this
optimization is added. I added another test that is the same,
but is updated with the new expected CFG.</div>
<div><br>
</div>
<div>Don't know what is the best thing to do with the original
"feature.ll" test in the case this patch gets accepted. Should
we remove it or keep it as a reference?</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Marcello</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>