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

Diogo N. Sampaio via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 2 03:38:41 PDT 2018


dnsampaio 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);
----------------
spatel wrote:
> 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.
Hi Sanjay,
So all expensive operations have been eliminated, I do not see why it wouldn't fit in InstCombne. We detect a pattern and we reduce it.

The transformation won't fit EarlyCSE pass,  as it might be required to move the producer `and` up, such as in:
```
define void @shift_r1(i32 %x) {
%sh = shl i32 %x, 8
%r2 = and i32 %sh, 44032
tail call void @use(i32 %r2)
%r1 = and i32 %x, 172
tail call void @use(i32 %r1)
ret void
}
```
We must move `%r1` before `%sh`. 

I could very well do a stand-alone BasicBlockPass, but don't think it would have a very appealing  reason to be.


https://reviews.llvm.org/D49229





More information about the llvm-commits mailing list