[PATCH] D129844: [InstCombine] Restrict "(X & 0xFF00) + xx00 --> (X + xx00) & 0xFF00"

[Jay Foad via Phabricator via llvm-commits]

foad added a reviewer: lebedev.ri.
foad added a subscriber: lattner.
foad added a comment.

My preference would be to completely remove this combine if possible (@piotr already knows this since we discussed it offline).

It was originally implemented by @lattner in https://github.com/llvm/llvm-project/commit/bff91d9a2e556b1aadf274c563cec80a483725a4 with the comment "This comes up when doing adds to bitfield elements" and a reference to this test in `test/Transforms/InstCombine/and.ll`:

  define i8 @test27(i8 %A) {
    %B = and i8 %A, 4
    %C = sub i8 %B, 16
    %D = and i8 %C, -16
    %E = add i8 %D, 16
    ret i8 %E
  }

So it looks like the intention is to be able to cancel out the ADD and the SUB in `((B - 16) & -16) + 16` by pushing one of them into the AND. But if this important then we could also achieve it by pulling one of them out of the AND, i.e. doing the opposite combine with a patch like this:

  diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  index a8f2cd79830a..998aae20c9f7 100644
  --- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  +++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  @@ -1843,6 +1843,13 @@ Instruction *InstCombinerImpl::visitAnd(BinaryOperator &I) {
           Value *NewAnd = Builder.CreateAnd(X, Op1);
           return BinaryOperator::CreateXor(NewAnd, Op1);
         }
  +
  +      // (X + 16) & -4 --> (X & -4) + 16
  +      if (Op0->hasOneUse() &&
  +          C->isNegative() && C->isShiftedMask() && *AddC == (*AddC & *C)) {
  +        Value *NewAnd = Builder.CreateAnd(X, Op1);
  +        return BinaryOperator::CreateAdd(NewAnd, ConstantInt::get(Ty, *AddC));
  +      }
       }
   
       // ((C1 OP zext(X)) & C2) -> zext((C1 OP X) & C2) if C2 fits in the

The advantage of doing it this way round is that it won't interfere with "base + constant offset" patterns that might be matched as addressing modes.

What's a good way of evaluating the effect of patches like this on codegen? The CodeGen lit tests aren't affected because they don't run InstCombine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129844/new/

https://reviews.llvm.org/D129844