[PATCH] D121355: [SelectionDAG] Fold shift constants into cmp

[Allen zhong via Phabricator via llvm-commits]

Allen marked 3 inline comments as done.
Allen added a comment.

In D121355#3384841 <https://reviews.llvm.org/D121355#3384841>, @craig.topper wrote:

> ARMTargetLowering::getARMCmp and AArch64TargetLowering::getAArch64Cmp both have code that tries to adjust the condition code to get the immediate to fit. So it looks like the primary problem is that introduction of the ISD::SRL that blocks the target from seeing a constant they can fix it.

hi, @craig.topper
Yes, ISD::SRL already support the 2^N  (isLegalAddImmediate will return false), but SelectionDAG still
fail to match it as it convert the 2^N to (2^N +1)/(2^N -1) , which is not a legal add immediate.
This is because we always do the transform：**Canonicalize GE/LE comparisons to use GT/LT comparisons**



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4300
+        // For the const C1 is equal to 2^n, which is legal for ICmp, if the
+        // type is not opaque, so this transform will bring in C equal to
+        // (2^n - 1), which is not legal for ICmp. Then, on the next iteration,
----------------
craig.topper wrote:
> it's not the type that is opaque, it's the constant value.
thanks, update comment


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4553
             !TLI.shouldAvoidTransformToShift(ShValTy, ShiftBits)) {
-          SDValue Shift = DAG.getNode(ISD::SRL, dl, ShValTy, N0,
-                                      DAG.getConstant(ShiftBits, dl, ShiftTy));
-          SDValue CmpRHS = DAG.getConstant(NewC, dl, ShValTy);
-          return DAG.getSetCC(dl, VT, Shift, CmpRHS, NewCond);
+          if (isLegalICmpImmediate(NewC.getSExtValue() << ShiftBits)) {
+            // Single instruction can describe as (NewC << ShiftBits) is legal
----------------
craig.topper wrote:
> Why does the new transform only happen if shouldAvoidTransformToShift returns false? You aren't creating a shift instruction.
good catch, thanks.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:4555
+            // Single instruction can describe as (NewC << ShiftBits) is legal
+            // (X >> ShiftBits) <  NewC     -> X <  (NewC << ShiftBits)
+            // (X >> ShiftBits) >= NewC     -> X >= (NewC << ShiftBits)
----------------
craig.topper wrote:
> This comment makes it look like the pattern (X >> ShiftBits) >= NewC is being matched, but there is no SRL instruction using X is there?
thanks, update comment


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

https://reviews.llvm.org/D121355