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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

1. Please always upload all patches with the full context (`-U99999`)
2. I think this was said in the other review, but while i acknowledge the missing fold, i'm not convinced that this approach is the right one. I think this should be done in smaller, fine-grained steps. I think, first one would be to canonicalize the `and`-mask before shifts. https://rise4fun.com/Alive/rOb Incidentally, that would already solve these, at least at -O3: https://godbolt.org/g/CkJzQd



================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2838
+
+  for (auto OtherUserV : MaskedValue->users()) {
+    BinaryOperator *OtherUser = dyn_cast<BinaryOperator>(OtherUserV);
----------------
dnsampaio wrote:
> spatel wrote:
> > dnsampaio wrote:
> > > lebedev.ri wrote:
> > > > Is this even for instcombine?
> > > > I wonder if this should be aggressiveinstcombine, or something else?
> > > If it is due the running costs, that is fixed. If is due the complexity of it ... I believe it is quite straight forward no?
> > Right - you'll notice that in my potential suggestions in D48278, I did *not* include instcombine. I think this has the same problem as that patch, but when you do it in instcombine, it gets multiplied by another 8x factor because we run instcombine so many times in the normal opt pass pipelines.
> > 
> > In general, we don't walk user lists in instcombine because that's not efficient (same concern that @efriedma raised in the other patch I think). On top of that, this patch is using recursive value tracking which is also expensive.
> > 
> > I suggest looking at (new)-gvn or early-cse to see if we can find the redundant op efficiently.
> No problems. Can remove both value tracking and iteration as to work in cases where the masked value only has 2 users, an and and a shift operation.
No the point. What @spatel said:
> In general, we don't walk user lists in instcombine because that's not efficient (same concern that @efriedma raised in the other patch I think).
We simply don't.
For this to be in instcombine, you'd need to match the `%4 = or i32 %2, %3` and look at it's parents.

Or maybe this should be in some other pass.



================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1416
+    return Shift;
   // See if we can simplify any instructions used by the instruction whose sole
   // purpose is to compute bits we don't care about.
----------------
Newline


================
Comment at: test/Transforms/InstCombine/FoldRedundantShiftedMasks.ll:26-31
+entry:
+  %0 = sext i16 %a to i32
+  %1 = shl i32 %0, 8
+  %2 = and i32 %0, 172
+  %3 = and i32 %1, 44032
+  %4 = or i32 %2, %3
----------------
Please cleanup the tests a little, prefix all these numeric variables with explicit `tmp` prefix.


https://reviews.llvm.org/D49229