[BUG] Fix regression in the instruction combiner after r214385

[majnemer]

Summary

r214385 fixed PR20189, but introduced a regression in the instruction combiner in cases where sub is used for immediates:

with r214385

sub nsw %x, 123  ====> add %x, -123

losing nsw/nuw attributes.

This patch fixes such a behavior for non-binary operations.

[zinovy.nis]

BTW, why to replace "SUB X, Imm" with "ADD X, -Imm"? Are there any benifits?

[zinovy.nis]

If SUB to ADD transformation is not required, my patch can be simplified to

// If this is a 'B = x-(-A)', change to B = x+A.
  if (const auto *BO = dyn_cast<BinaryOperator>(Op1)) {
    if (Value *V = dyn_castNegVal(Op1)) {
      BinaryOperator *Res = BinaryOperator::CreateAdd(Op0, V);
      assert(BO->getOpcode() == Instruction::Sub &&
             "Expected a subtraction operator!");
      if (BO->hasNoSignedWrap() && I.hasNoSignedWrap())
        Res->setHasNoSignedWrap(true);
      return Res;
    }
  }

David, how do you think?

[majnemer]

Your preservation of nsw is wrong for the following case:

%x i16 = 32768

sub nsw %x, 32768 will be 0.
add nsw %x, -32768 will be undef.

Your preservation of nuw is wrong for the following case:

%x i16 = 63488

sub nuw %x, 8 will be 63480.
add nuw %x, -8 will be undef.

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1542–1545

The following should work better:

} else {
  const auto *C = cast<Op1>(Constant);
  if (!C->isMinSignedValue() && I.hasNoSignedWrap())
    Res->setHasNoSignedWrap(true);

[majnemer]

I've committed a variant of my change in rL216269, thanks for bringing this regression to my attention.

[zinovy.nis]

David, thanks for your commit.

But why we need to replace SUB with ADD in LLVM IR? I see no reasons for it. Or may be there's a pass which strictly needs namely ADD and fails to deal with SUB? If not, I'd prefer apply a patch from my first comment:

// If this is a 'B = x-(-A)', change to B = x+A.
  if (const auto *BO = dyn_cast<BinaryOperator>(Op1)) {
    if (Value *V = dyn_castNegVal(Op1)) {
      BinaryOperator *Res = BinaryOperator::CreateAdd(Op0, V);
      assert(BO->getOpcode() == Instruction::Sub &&
             "Expected a subtraction operator!");
      if (BO->hasNoSignedWrap() && I.hasNoSignedWrap())
        Res->setHasNoSignedWrap(true);
      return Res;
    }
  }

[zinovy.nis]

BTW, David can you please post your patches for a review in Phabricator first, before committing them? So other LLVM community members could have a chance of estimating an impact of it for their projects. Thanks :-)

[majnemer]

Choosing a single canonical form allows LLVM to reduce complexity in other areas.

Otherwise, other parts of LLVM would have to deal with two cases: normal add and sub with an operand which is always negative.

lib/Transforms/InstCombine/InstCombineAddSub.cpp

Instruction *InstCombiner::visitSub(BinaryOperator &I) {
   if (Value *V = dyn_castNegVal(Op1)) {
     BinaryOperator *Res = BinaryOperator::CreateAdd(Op0, V);
 
     if (const auto *BO = dyn_cast<BinaryOperator>(Op1)) {
       assert(BO->getOpcode() == Instruction::Sub &&
              "Expected a subtraction operator!");
       if (BO->hasNoSignedWrap() && I.hasNoSignedWrap())
         Res->setHasNoSignedWrap(true);
+    } else {
+      // Preserve nsw/nuw for non-binary operations.
+      Res->setHasNoSignedWrap(I.hasNoSignedWrap());
+      Res->setHasNoUnsignedWrap(I.hasNoUnsignedWrap());
     }
 
     return Res;
   }
 
   if (I.getType()->isIntegerTy(1))
     return BinaryOperator::CreateXor(Op0, Op1);