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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2024
     return replaceInstUsesWith(I, V);
+  { // Fold redundant masking.
+    const APInt *AndC, *ShAndC, *ShftAmt;
----------------
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.
```


================
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))),
----------------
You could change that into `m_c_Or` to cover the commutative case too. Then you can get rid of the code duplication.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2031
+        (X1 == X2)) {
+      BinaryOperator *And = cast<BinaryOperator>(I.getOperand(0));
+      BinaryOperator *ShtAnd = cast<BinaryOperator>(I.getOperand(1));
----------------
You could replace the lines 2031-2035 with
```
BinaryOperator *Op0 = cast<BinaryOperator>(I.getOperand(0));
BinaryOperator *Op1 = cast<BinaryOperator>(I.getOperand(1));
       
BinaryOperator *And, *Shift;
if ((Shift = dyn_cast<BinaryOperator>(Op0->getOperand(0))))
  And = Op1;
else if ((Shift = dyn_cast<BinaryOperator>(Op0->getOperand(1))))
  And = Op1;
else if ((Shift = dyn_cast<BinaryOperator>(Op1->getOperand(0))))
  And = Op0;
else
  And = Op0;
```
and then remove the commutative pattern match from below (lines 2049-2070).


================
Comment at: test/Transforms/InstCombine/FoldRedundantShiftedMasks.ll:4
+
+; https://reviews.llvm.org/D48278
+; Fold redundant masking operations of shifted value
----------------
The reference to Phabricator is confusing. Please remove it. It'll become irrelevant with time anyway.


https://reviews.llvm.org/D49229