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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some more comments.
Please enhance test coverage, and note the note about possible miscompile.

In https://reviews.llvm.org/D48828#1167921, @inouehrs wrote:

> > ! In https://reviews.llvm.org/D48828#1166791, @lebedev.ri wrote:
> >  Can you explain that in layman terms?
> >  I don't see anything phi-related in instsimplify changes.
>
> To help jump threading, actually we need to optimize the code sequence spanning multiple BBs. For an example of `shl->or->and` case looks like
>
>   BB1:
>     %shl = shl nuw i64 %val, 32
>     %or = or i64 %shl, 1
>     br %BB2
>   BB2:
>     %phi = phi i64 [ %or, %BB1 ], ... 
>     %and = and i64 %phi, 1
>
>
> The current instcombine cannot optimize such cases.
>  My instsimplify patch does not handle phi by itself. The NewGVN calls instsimplify to check opportunities for simplifying instructions over phi.


Aha, that is the bit i was looking for. Please update the differential's description with that.



================
Comment at: lib/Analysis/InstructionSimplify.cpp:1814
+  // from X and Y are disjoint in (X << A) | Y.
+  // ((X << A) | Y) & 1 -> Y  if effective width of Y is 1 (i.e. boolean).
+  // ((X << A) | Y) & (1 << A) -> X << A  if effective width of X is 1.
----------------
inouehrs wrote:
> lebedev.ri wrote:
> > I do not understand.
> > Why is this only handling the case where `Y` is `bool`?
> > 
> I made the code more generic. What we really need to check is that this AND op selects all bits of X or Y, and no bit from the another.
> 
> What we really need to check is that this AND op selects all bits of X or Y, and no bit from the another.

That was exactly my point :)


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1287-1288
+    unsigned EffWidthY = Width - YKnown.countMinLeadingZeros();
+    if (EffWidthY <= ShiftCnt)
+      return X;
+  }
----------------
Please add some negative test where this fails.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1823
+      match(Op0, m_c_Or(m_NUWShl(m_Value(X), m_APInt(ShAmt)), m_Value(Y)))) {
+    APInt Mask = cast<ConstantInt>(Op1)->getValue();
+    unsigned ShiftCnt = ShAmt->getZExtValue();
----------------
I think you can use `const APInt& Mask` here.


================
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);
----------------
But it seems like this only supports extraction of a bit-wide values?
Please at least add a `FIXME` comment then.


================
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
----------------
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.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:1843
+            return cast<Instruction>(Op0)->getOperand(0);
+        }
+      }
----------------
This also needs //some// negative tests.


================
Comment at: test/Transforms/InstSimplify/AndOrXor.ll:994
+  ret i64 %5
+}
----------------
Please also add two tests with `i32 | i32` - test extracting low part, and test high part.


https://reviews.llvm.org/D48828