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

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked 11 inline comments as done.
inouehrs added inline comments.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1825
+    unsigned ShiftCnt = ShAmt->getZExtValue();
+    if (Mask == 1 || Mask == (1uLL << ShiftCnt)) {
+      KnownBits YKnown = computeKnownBits(Y, Q.DL, 0, Q.AC, Q.CxtI, Q.DT);
----------------
lebedev.ri wrote:
> But it seems like this only supports extraction of a bit-wide values?
> Please at least add a `FIXME` comment then.
Actually, this is an unnecessary check. I removed it to increase the optimization opportunities.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1832-1833
+        unsigned EffWidthX = Width - XKnown.countMinLeadingZeros();
+        uint64_t EffBitsY = (1uL << EffWidthY) - 1;
+        uint64_t EffBitsX = ((1uL << EffWidthX) - 1) << ShiftCnt;
+        // If the mask is extracting all bits from X or Y as is, we can skip
----------------
lebedev.ri wrote:
> Aha. This will miscompile if you are operating on types wider than `i64`.
> 
> Please add tests with wider types (`i128`, with elements of `i64`, e.g.),
> and use `APInt` in these calculations.
Right. Fixed using APInt.


================
Comment at: test/Transforms/InstSimplify/AndOrXor.ll:994
+  ret i64 %5
+}
----------------
lebedev.ri wrote:
> Please also add two tests with `i32 | i32` - test extracting low part, and test high part.
More tests (including negative tests) added.


https://reviews.llvm.org/D48828