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

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio added a comment.

Fixed execution costs. See below.



================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2838
+
+  for (auto OtherUserV : MaskedValue->users()) {
+    BinaryOperator *OtherUser = dyn_cast<BinaryOperator>(OtherUserV);
----------------
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?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2838
+
+  for (auto OtherUserV : MaskedValue->users()) {
+    BinaryOperator *OtherUser = dyn_cast<BinaryOperator>(OtherUserV);
----------------
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.


https://reviews.llvm.org/D49229