[PATCH] D48828: [InstSimplify] fold extracting from std::pair

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Hmm, this looks about right.
`SimplifyRightShift()` change looks good,
but i have hopefully a last portion of comments on the `SimplifyAndInst()`..



================
Comment at: lib/Analysis/InstructionSimplify.cpp:1828-1830
+  Value *Y;
+  if (isa<ConstantInt>(Op1) &&
+      match(Op0, m_c_Or(m_NUWShl(m_Value(X), m_APInt(ShAmt)), m_Value(Y)))) {
----------------
```
Value *Y, *Shift;
if (isa<ConstantInt>(Op1) &&
    match(Op0, m_c_Or(m_CombineAnd(m_NUWShl(m_Value(X), m_APInt(ShAmt)),
                                   m_Value(Shift)),
                      m_Value(Y)))) {
}
```


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1840
+      const unsigned EffWidthX = Width - XKnown.countMinLeadingZeros();
+      const APInt EffBitsY = (APInt(Width, 1) << EffWidthY) - 1;
+      const APInt EffBitsX = ((APInt(Width, 1) << EffWidthX) - 1) << ShiftCnt;
----------------
Pedantically, i still somehow don't like these calculations :/
I think at least this should be:
```
const APInt EffBitsY = APInt::getLowBitsSet(Width, EffWidthY);
```
Not sure how to more nicely express the `EffBitsX`.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1844-1846
+      if ((Mask & EffBitsY) == EffBitsY && (Mask & EffBitsX) == 0)
+        return Y;
+      if ((Mask & EffBitsX) == EffBitsX && (Mask & EffBitsY) == 0) {
----------------
Please see if `APInt::intersects()` and `APInt::isSubsetOf()` could be used here.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1847-1850
+        if (cast<Instruction>(Op0)->getOperand(0) == Y)
+          return cast<Instruction>(Op0)->getOperand(1);
+        else
+          return cast<Instruction>(Op0)->getOperand(0);
----------------
```
return Shift;
```


================
Comment at: test/Transforms/InstSimplify/AndOrXor.ll:973
+;
+  %1 = zext i32 %a to i64
+  %2 = zext i1 %b to i64
----------------
Please give names to all these variables in all the tests, prefix the numeric id with tmp - `s/%/%tmp/`.


================
Comment at: test/Transforms/InstSimplify/AndOrXor.ll:1078-1091
+define i64 @shl_or_and8(i64 %a, i1 %b) {
+; CHECK-LABEL: @shl_or_and8(
+; CHECK-NEXT:    [[TMP1:%.*]] = zext i1 [[B:%.*]] to i128
+; CHECK-NEXT:    [[TMP2:%.*]] = trunc i128 [[TMP1]] to i64
+; CHECK-NEXT:    ret i64 [[TMP2]]
+;
+  %1 = zext i64 %a to i128
----------------
Could you please move this to before the negative tests?


https://reviews.llvm.org/D48828