[PATCH] D49229: [InstCombine] Fold redundant masking operations of shifted value

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 30 08:11:33 PDT 2018


spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2058
+      Builder.CreateBinOp(Opcode, And, ConstantInt::get(I.getType(), *ShftAmt));
+  DeadAnd->replaceAllUsesWith(NewShift);
+  return BinaryOperator::CreateOr(And, NewShift);
----------------
dnsampaio wrote:
> dnsampaio wrote:
> > lebedev.ri wrote:
> > > But how do you know it's dead? I don't think you checked that it is one-use?
> > > And if it is one-use, then it will be dead after the parent instruction will be replaced, so why do we care?
> > > I guess you want to add a bit more tests.
> > The new shift, being created, holds the same value of the DeadAnd. So we can just replace all uses of the DeadAnd with the new shifted value. Test added, to be uploaded.
> > 
> Perhaps the naming is misleading. It is "dead" in the sense that the new shift holds the same value, and all it's uses must be replaced. But no, it is not restricted to be a single use. Adding example test.
I'll say it again: this is getting weird because we're trying to make instcombine do something it should not be doing. 

You crippled the transform to try to make it fit, and now you're trying to expand it to handle the motivating cases.

The most basic case where this transform should fire looks like this:

```
define void @shift_r1(i32 %x) {
  %r1 = and i32 %x, 172
  %sh = shl i32 %x, 8
  %r2 = and i32 %sh, 44032
  tail call void @use(i32 %r1)
  tail call void @use(i32 %r2)
  ret void
}

-->
define void @shift_r1(i32 %x) {
  %r1 = and i32 %x, 172
  %r2 = shl i32 %r1, 8
  tail call void @use(i32 %r1)
  tail call void @use(i32 %r2)
  ret void
}

```

There is no 'or' in the pattern. The optimization is about recognizing a common subexpression and using it to remove an instruction. That could be CSE, GVN, or a standalone pass. I don't see how this fits in instcombine.


https://reviews.llvm.org/D49229





More information about the llvm-commits mailing list