[PATCH] D62818: [InstCombine] Allow ((X << Y) & SignMask) != 0 to be optimized as (X << Y) s< 0.

Mon Jun 3 15:27:24 PDT 2019

lebedev.ri added a comment.

I'm about to post dagcominer undo-fold, hold on..

In D62818#1528149 <https://reviews.llvm.org/D62818#1528149>, @efriedma wrote:

> On the instcombine side, one thing worth noting which isn't called out in the commit message is the interaction with other instcombine patterns.  In the testcase, note that the final IR actually doesn't contain any mask; instead, it checks `icmp slt i32 [[SHL]], 0`.  Huihui, please update the commit message to make this clear.
>
> It's possible we should also implement the related pattern to transform `(x & (signbit >> y)) != 0` to `(x << y) < 0`, sure.

Yes, now that would be a good patch, +see inline comment.

> In terms of whether it's universally profitable, I'm not sure... I guess if somehow "icmp ne X, 0" is free, but "icmp slt X, 0" isn't, it could be an issue, but I don't think that applies to any architecture I can think of.

I think there may or may not bea confusion here. We are in a middle-end here. Other than TTI,
we don't really care about what ever backed/target may find troubling/unprofitable.
We only care about producing most simple IR, that is most suited for further transforms.
That new IR may, or may not, be optimal for any particular target.
If IR is not optimal for back-end, then an opposite transform should be present in backend.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1606-1611
   // Turn ((X >> Y) & C2) == 0  into  (X & (C2 << Y)) == 0.  The latter is
   // preferable because it allows the C2 << Y expression to be hoisted out of a
   // loop if Y is invariant and X is not.
   if (Shift->hasOneUse() && C1.isNullValue() && Cmp.isEquality() &&
-      !Shift->isArithmeticShift() && !isa<Constant>(Shift->getOperand(0))) {
+      !C2.isSignMask() && !Shift->isArithmeticShift() &&
+      !isa<Constant>(Shift->getOperand(0))) {
----------------
There should also be a sibling fold with swapped shift directions

================
Comment at: test/Transforms/InstCombine/icmp-shl-and.ll:12-13
+  %shl = shl i32 %y, %sub
+  %and = and i32 %shl, -2147483648
+  %tobool = icmp ne i32 %and, 0
+  %selv = select i1 %tobool, i32 %a, i32 %b
----------------
Hmm, this already should be folding: https://godbolt.org/z/77mvnv
I guess the order of folds is wrong.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62818/new/

https://reviews.llvm.org/D62818