[PATCH] D48828: [InstSimplify] fold extracting from std::pair
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 25 12:43:58 PDT 2018
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
More information about the llvm-commits
mailing list