[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;
----------------
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.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2031
+        (X1 == X2)) {
+      BinaryOperator *And = cast<BinaryOperator>(I.getOperand(0));
+      BinaryOperator *ShtAnd = cast<BinaryOperator>(I.getOperand(1));
----------------
labrinea wrote:
> 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).
I made a mistake at the else clause. Should be
```
else {
  Shift = cast<BinaryOperator>(Op1->getOperand(1));
  And = Op0;
}
```


https://reviews.llvm.org/D49229