This is an archive of the discontinued LLVM Phabricator instance.

[BUG] Fix regression in the instruction combiner after r214385
AbandonedPublic

Authored by majnemer on Aug 21 2014, 7:00 AM.

Details

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.

Diff Detail

Event Timeline

zinovy.nis updated this revision to Diff 12767.Aug 21 2014, 7:00 AM
zinovy.nis retitled this revision from to [BUG] Fix regression after r214385.
zinovy.nis updated this object.
zinovy.nis edited the test plan for this revision. (Show Details)
zinovy.nis added reviewers: majnemer, bkramer.
zinovy.nis set the repository for this revision to rL LLVM.
zinovy.nis retitled this revision from [BUG] Fix regression after r214385 to [BUG] Fix regression in the instruction combiner after r214385.
zinovy.nis updated this object.
zinovy.nis added a subscriber: Unknown Object (MLST).

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

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 edited edge metadata.Aug 22 2014, 8:38 AM

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);

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

zinovy.nis added a comment.EditedAug 25 2014, 12:15 AM

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;
    }
  }

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 :-)

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.

majnemer commandeered this revision.Sep 21 2014, 11:42 AM
majnemer abandoned this revision.
majnemer edited reviewers, added: zinovy.nis; removed: majnemer.