[PATCH] D63391: [CodeGen] [SelectionDAG] More efficient code for X % C == 0 (UREM case) (try 2)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: include/llvm/CodeGen/TargetLowering.h:4069-4073
+  SDValue PrepareUREMEqFold(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
+                            DAGCombinerInfo &DCI, const SDLoc &DL,
+                            SmallVectorImpl<SDNode *> &Created) const;
+  SDValue BuildUREMEqFold(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
+                          DAGCombinerInfo &DCI, const SDLoc &DL) const;
----------------
Formatting: use the current guideline 'lowerCamelCase' on these function names?


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4433-4434
+  // If MUL is unavailable, we cannot proceed in any case
+  if (!(DCI.isBeforeLegalizeOps() ? isOperationLegalOrCustom(ISD::MUL, REMVT)
+                                  : isOperationLegal(ISD::MUL, REMVT)))
+    return SDValue();
----------------
Not sure if this legality check is necessary to avoid infinite looping and/or pick up extra diffs, but we don't normally want to distinguish 'custom' from 'legal' like this.

Can we use this simpler check?
  if (!isOperationLegalOrCustom(ISD::MUL, REMVT)) 
    return SDValue()

That is, it doesn't matter what stage of combining we're in. If the target supports MUL for this type, this is always a good transfom for performance. That might allow removing the earlier 'isTypeLegal()' check too.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:4477-4478
+    // We need ROTR to do this
+    if (!(DCI.isBeforeLegalizeOps() ? isOperationLegalOrCustom(ISD::ROTR, REMVT)
+                                    : isOperationLegal(ISD::ROTR, REMVT)))
+      return SDValue();
----------------
See earlier comment about legality of the MUL. Can we use the simpler check independent of combining stage?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63391