[PATCH] Add NSW/NUW flags in InstCombine

[Duncan P. N. Exon Smith]

On 2013 Dec 17, at 08:20, Joey Gouly <joey.gouly at arm.com> wrote:

> Add the NSW or NUW flags in cases that we can detect will not produce an overflow.
> 
> Patch by Georgia Kouveli!
> 
> http://llvm-reviews.chandlerc.com/D2426
> 
> Files:
>  lib/Transforms/InstCombine/InstCombine.h
>  lib/Transforms/InstCombine/InstCombineAddSub.cpp
>  lib/Transforms/InstCombine/InstructionCombining.cpp
>  test/Transforms/InstCombine/cast.ll
>  test/Transforms/InstCombine/ffs-1.ll
>  test/Transforms/InstCombine/rem.ll
>  test/Transforms/InstCombine/select.ll
>  test/Transforms/InstCombine/sext.ll
>  test/Transforms/InstCombine/zext-bool-add-sub.ll
> <D2426.1.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Hi Joey/Georgia,

Thanks for the patch!

I have a few points regarding the implementation details of the patch, but first:  I’m not clear on the benefit.  AFAIK, the nuw/nsw flags just serve to provide poison values when overflow leads to undefined behavior.  If we know these operations won’t overflow, why add the flags?

In other words, while the transformations are correct, I’m not convinced this is a good idea (although I could be missing something!).  If you send the patch back, please point out what I’m missing here.

Assuming I’m wrong, here are a few points about the patch itself:

1. InstCombiner::WillNotOverflowSignedAddSub and InstCombiner::WillNotOverflowSignedAdd:

   if (ComputeNumSignBits(LHS) > 1 && ComputeNumSignBits(RHS) > 1)
     return true;
 
+  // If both LHS and RHS can be represented as signed numbers with fewer bits
+  // than their type, then the addition will not overflow.
+  if (WillNotOverflowSignedAddSub(LHS, RHS))
+    return true;

It looks to me like WillNotOverflowSignedAddSub is subsumed under the preceding if statement.

Given that, perhaps WillNotOverflowSignedAddSub should be implemented as a one-liner:

    return ComputeNumSignBits(LHS) > 1 && ComputeNumSignBits(RHS) > 1;

Then, you can replace the preceding "if (ComputeNumSignBits, etc.)" with a call to your new function.

2. InstCombiner::WillNotOverflowUnsignedAdd:

+  IntegerType *SmallerType = IntegerType::get(LHS->getType()->getContext(),
+                                             OrigTypeBits-1);
+  ConstantInt *CLHS = dyn_cast<ConstantInt>(LHS);
+  bool IsLHSOk = (CLHS && CLHS->getBitWidth() <= 64 &&
+                  CLHS->getBitWidth() <= 64 &&
+                  ConstantInt::isValueValidForType(SmallerType,
+                                                   CLHS->getZExtValue()))
+                 || isa<ZExtInst>(LHS);

If I understand correctly, since this is unsigned, you’re really checking whether the MSB is 0.  You can use llvm::ComputeSignBit to calculate this more generally:

    bool KnownZero, KnownOne;
    ComputeSignBit(LHS, KnownZero, KnownOne, TD);
    if (!KnownZero)
      return false;
    ComputeSignBit(RHS, KnownZero, KnownOne, TD);
    return KnownZero;

3. In InstCombiner::visitGetElementPtrInst:

         Sum = Builder->CreateAdd(SO1, GO1, PtrOp->getName()+".sum");
+        if (BinaryOperator *SumBO = dyn_cast<BinaryOperator>(Sum)) {
+          SumBO->setHasNoSignedWrap(WillNotOverflowSignedAdd(SO1, GO1));
+          SumBO->setHasNoUnsignedWrap(WillNotOverflowUnsignedAdd(SO1, GO1));
+        }

The signature of IRBuilder::CreateAdd looks like this:

  Value *CreateAdd(Value *LHS, Value *RHS, const Twine &Name = "",
                   bool HasNUW = false, bool HasNSW = false) {

So, you can just say:

    Sum = Builder->CreateAdd(SO1, GO1, PtrOp->getName()+".sum”,
                             WillNotOverflowUnsignedAdd(SO1, GO1),
                             WillNotOverflowSignedAdd(SO1, GO1));

4. Finally, while you have updated old tests, you should also add new tests against the new features you’ve added.

Duncan