[llvm-commits] PATCH: Preserving the 'nsw' flag in the instruction combiner.

[Nick Lewycky]

On 10 August 2011 06:07, Pranav Bhandarkar <pranavb at codeaurora.org> wrote:

> Hi,
>
> Subsequent to a discussion on the 'nsw' flag on llvm dev
> (http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-June/040856.html), I have
> written a small function that allows the instruction combiner to preserve
> the 'nsw' flag under certain conditions. Preserving this flag allows
> optimization passes down the pipeline to be more aggressive.
>
> For instance, consider this synthetic test case.
> int foo (int x)
> {     int j = x + 2;
>      int y = j + 3;
>
>      if (j < y) return 1;
>      else return 0;
> }
> This can be reduced to always return 1. However LLVM does not do this
> transformation. Consider the following piece of code
>
> 1 %add = add nsw i32 %tmp, 2
> 2 %add3 = add nsw i32 %add, 3
> 3 %cmp = icmp slt i32 %add, %add3
> 4 br i1 %cmp, label %if.then, label %if.else
>
> Here add = tmp + 2 and add3 is effectively tmp + 5 and the comparison "slt
> %add, %add3" is always true. However, the instruction combiner combines
> instructions 1 and 2 to produce
>
> 2 %add3 = add i32 %tmp, 5
>
> Note the 'nsw' flag is dropped because the combiner does not reason about
> the applicability of the 'nsw' flag in the new combined instruction. If now
> the simplifycfg pass is run it does not simplify the comparison because one
> of its operands has the 'nsw' flag while the other does not. The attached
> patch checks the resulting constant in the new instruction for overflow. If
> not, it keeps the flag. This allows the "simplifycfg" pass to simplify the
> comparison and reduces the above function to return 1.
>

Hi Pranav,

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?

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.

Then I have some comments on your patch. This code:

+  if (!isa<Constant>(B) || !isa<Constant>(C) || !isa<Constant>(V)) {
+    return false;
+  }
[...]

+
+  ConstantInt *CV = dyn_cast<ConstantInt>(V);
+  ConstantInt *CB = dyn_cast<ConstantInt>(B);
+  ConstantInt *CC = dyn_cast<ConstantInt>(C);
+
+  if (!CV || !CB || !CC) {
+    return false;
+  }

is redundant. You don't need the first test because the second one will
catch a superset of all the cases.

+ if (!I.isAssociative()) {

+    return false;
+  }
+
+  // We will not reason about Mul for now.
+  if (Opcode == Instruction::Mul) {
+    return false;
+  }

How about just "if (Opcode != Instruction::Add && Opcode !=
Instruction::Sub) { return false; }"? That makes it clear that the function
only handles add and sub.

Nick

I have three attachments - The patch, a testcase that should be added to
> test/Transforms/InstCombine and my regression report. This patch was
> successfully regression tested on x86_64.
>
> Pranav
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110810/0b9fd2c3/attachment.html>