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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2024
     return replaceInstUsesWith(I, V);
+  { // Fold redundant masking.
+    const APInt *AndC, *ShAndC, *ShftAmt;
----------------
labrinea wrote:
> samparker wrote:
> > labrinea wrote:
> > > You could add a more descriptive comment like:
> > > ```// Fold redundant masking.
> > > // Let C3 = C1 lsh C2
> > > // (X & C1) | ((X lsh C2) & C3) -> (X & C1) | ((X & C1) lsh C2)
> > > // Also handles the commutative cases.
> > > ```
> > Looks like the convention here is to perform more specific folding after the calls to SimplifyAssociativeOrCommutative, etc.. And there is already a case for '(A & C)|(B & D)'.
> Yeap, Sam is right. Line 2079 of the original source.
Just put this into a new function and call it from here.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2026-2027
+    const APInt *AndC, *ShAndC, *ShftAmt;
+    Value *X1, *X2;
+    if (match(&I, m_Or(m_c_And(m_Value(X1), m_APInt(AndC)),
+                       m_c_And(m_OneUse(m_Shift(m_Value(X2), m_APInt(ShftAmt))),
----------------
s/`X1`/`X`/


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2027
+    Value *X1, *X2;
+    if (match(&I, m_Or(m_c_And(m_Value(X1), m_APInt(AndC)),
+                       m_c_And(m_OneUse(m_Shift(m_Value(X2), m_APInt(ShftAmt))),
----------------
samparker wrote:
> labrinea wrote:
> > You could change that into `m_c_Or` to cover the commutative case too. Then you can get rid of the code duplication.
> We already know I is an Or.
> if (match(&I, m_Or(m_c_And(m_Value(X1), m_APInt(AndC)),

@labrinea is correct.
This is backwards. `m_c_And` won't ever matter. Constant will be rhs, So it should be `m_And`.
But this isn't true about `m_Or`, that one will be missing some commutative cases. It should be m_c_Or.
(i thought i commented that already?)


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2028
+    if (match(&I, m_Or(m_c_And(m_Value(X1), m_APInt(AndC)),
+                       m_c_And(m_OneUse(m_Shift(m_Value(X2), m_APInt(ShftAmt))),
+                               m_APInt(ShAndC)))) &&
----------------
Same here, constant will always be RHS, no need for commutativity.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2028
+    if (match(&I, m_Or(m_c_And(m_Value(X1), m_APInt(AndC)),
+                       m_c_And(m_OneUse(m_Shift(m_Value(X2), m_APInt(ShftAmt))),
+                               m_APInt(ShAndC)))) &&
----------------
lebedev.ri wrote:
> Same here, constant will always be RHS, no need for commutativity.
s/`m_Value(X2)`/`m_Deferred(X)`/


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2030
+                               m_APInt(ShAndC)))) &&
+        (X1 == X2)) {
+      BinaryOperator *And = cast<BinaryOperator>(I.getOperand(0));
----------------
And this is no longer needed.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2049-2052
+    if (match(&I, m_Or(m_c_And(m_OneUse(m_Shift(m_Value(X2), m_APInt(ShftAmt))),
+                               m_APInt(ShAndC)),
+                       m_c_And(m_Value(X1), m_APInt(AndC)))) &&
+        (X1 == X2)) {
----------------
This should be
```
    if (match(&I, m_c_Or(m_And(m_OneUse(m_Shift(m_Value(X), m_APInt(ShftAmt))),
                               m_APInt(ShAndC)),
                         m_And(m_Deferred(X), m_APInt(AndC)))) {
```


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2053-2058
+      BinaryOperator *And = cast<BinaryOperator>(I.getOperand(1));
+      BinaryOperator *ShtAnd = cast<BinaryOperator>(I.getOperand(0));
+      BinaryOperator *Shift = cast<BinaryOperator>(
+          isa<BinaryOperator>(ShtAnd->getOperand(0)) ? ShtAnd->getOperand(0)
+                                                     : ShtAnd->getOperand(1));
+      const Instruction::BinaryOps Opcode = Shift->getOpcode();
----------------
Capture them in `match()`.


https://reviews.llvm.org/D49229