<div class="gmail_quote">On 10 August 2011 06:07, Pranav Bhandarkar <span dir="ltr"><<a href="mailto:pranavb@codeaurora.org">pranavb@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Hi,<br>
<br>
Subsequent to a discussion on the 'nsw' flag on llvm dev<br>
(<a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-June/040856.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-June/040856.html</a>), I have<br>
written a small function that allows the instruction combiner to preserve<br>
the 'nsw' flag under certain conditions. Preserving this flag allows<br>
optimization passes down the pipeline to be more aggressive.<br>
<br>
For instance, consider this synthetic test case.<br>
int foo (int x)<br>
{ int j = x + 2;<br>
int y = j + 3;<br>
<br>
if (j < y) return 1;<br>
else return 0;<br>
}<br>
This can be reduced to always return 1. However LLVM does not do this<br>
transformation. Consider the following piece of code<br>
<br>
1 %add = add nsw i32 %tmp, 2<br>
2 %add3 = add nsw i32 %add, 3<br>
3 %cmp = icmp slt i32 %add, %add3<br>
4 br i1 %cmp, label %if.then, label %if.else<br>
<br>
Here add = tmp + 2 and add3 is effectively tmp + 5 and the comparison "slt<br>
%add, %add3" is always true. However, the instruction combiner combines<br>
instructions 1 and 2 to produce<br>
<br>
2 %add3 = add i32 %tmp, 5<br>
<br>
Note the 'nsw' flag is dropped because the combiner does not reason about<br>
the applicability of the 'nsw' flag in the new combined instruction. If now<br>
the simplifycfg pass is run it does not simplify the comparison because one<br>
of its operands has the 'nsw' flag while the other does not. The attached<br>
patch checks the resulting constant in the new instruction for overflow. If<br>
not, it keeps the flag. This allows the "simplifycfg" pass to simplify the<br>
comparison and reduces the above function to return 1.<br></blockquote><div><br></div><div>Hi Pranav,</div><div><br></div><div>Thanks for working on this! Firstly, I have a high-level question: why can't (X +nsw (C1 +nsw C2) always become (X +nsw C3)? Your patch spends a lot of time verifying that overflow couldn't occur, but you're given an assumption a priori that it can't because the nsw flag is present. Sure it can overflow the unsigned wrap point (crossing between -1 to 0) but when merging two adds or two subs, does that ever matter?</div>
<div><br></div><div>If I'm right and you don't need to test for overflow then you should be able to extend this to nuw and exact too.</div><div><br></div><div>Then I have some comments on your patch. This code:</div>
<div><pre style="font-family: 'Times New Roman'; font-size: medium; word-wrap: break-word; white-space: pre-wrap; ">+ if (!isa<Constant>(B) || !isa<Constant>(C) || !isa<Constant>(V)) {
+ return false;
+ }
[...]</pre><pre style="word-wrap: break-word; "><span class="Apple-style-span" style="font-family: 'Times New Roman'; white-space: pre-wrap; font-size: medium; ">+
+ ConstantInt *CV = dyn_cast<ConstantInt>(V);
+ ConstantInt *CB = dyn_cast<ConstantInt>(B);
+ ConstantInt *CC = dyn_cast<ConstantInt>(C);
+
+ if (!CV || !CB || !CC) {
+ return false;
+ }</span>
</pre>is redundant. You don't need the first test because the second one will catch a superset of all the cases.</div><div><br></div><div><span class="Apple-style-span" style="font-family: 'Times New Roman'; white-space: pre-wrap; font-size: medium; ">+ if (!I.isAssociative()) {</span></div>
<div><pre style="word-wrap: break-word; "><span class="Apple-style-span" style="font-family: 'Times New Roman'; white-space: pre-wrap; font-size: medium; ">+ return false;
+ }
+
+ // We will not reason about Mul for now.
+ if (Opcode == Instruction::Mul) {
+ return false;
+ }</span>
</pre>How about just "if (Opcode != Instruction::Add && Opcode != Instruction::Sub) { return false; }"? That makes it clear that the function only handles add and sub.</div><div><br></div><div>Nick</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
I have three attachments - The patch, a testcase that should be added to<br>
test/Transforms/InstCombine and my regression report. This patch was<br>
successfully regression tested on x86_64.<br>
<font color="#888888"><br>
Pranav<br>
</font><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>