[PATCH] D130862: [LegalizeTypes][WIP] Improve splitting for urem/udiv by constant for some constants.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D130862#3707290 <https://reviews.llvm.org/D130862#3707290>, @nickdesaulniers wrote:

> Do we want to maintain codegen'ing a libcall when optimizing for code size? If so, is there an existing test that demonstrates that, or do we need a new one?
>
> Does 32b MIPS need custom target lowering? 32b ppc?

I have not checked yet.

>> Support constants that have trailing zeros such as 10 and 12 using the formula suggested by Eli.
>
> Mind adding a citation via comment in source or commit message?

Yes. I need to update comments. Not sure if we want the trailing zeros case in the base patch or in its own commit. I squashed everything together after I changed the base patch interface so much to support DIV and DIVREM.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7264
+    RemL = DAG.getNode(ISD::ADD, dl, HiLoVT, RemL,
+                       DAG.getNode(ISD::AND, dl, HiLoVT, LL,
+                                   DAG.getConstant(Mask, dl, HiLoVT)));
----------------
nickdesaulniers wrote:
> Should this be `LL` or `DL`? (I dunno, just asking; curious why `DL` exists).
LL is correct. DL is the shifted version. We need the bits that were shifted off here so we need the original.

I need to improve variable naming.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7278
+  // Join the remainder halves.
+  SDValue Rem = DAG.getNode(ISD::BUILD_PAIR, dl, VT, RemL, RemH);
+
----------------
nickdesaulniers wrote:
> Every caller is `BUILD_PAIR`'ing the results. Would it be a better interface to simply return an `SDValue` for the remainder and one for the quotient, rather than a pair of `SDValue`s that need to be `BUILD_PAIR`ed again in the caller (even though we've done so already in the callee)?
The caller in LegalizeIntegerTypes does not BUILD_PAIR the results.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:20433
+      SDValue Rem = DAG.getNode(ISD::BUILD_PAIR, dl, VT, Result[2], Result[3]);
+      return DAG.getNode(ISD::MERGE_VALUES, dl, Op->getVTList(), {Div, Rem});
+    }
----------------
nickdesaulniers wrote:
> I find the Result param a little confusing. Sometimes it starts with the quotient, sometimes it starts with the remainder. Should these be two distinct parameters to `expandDIVREMByConstant`?
> 
> ie. one `SmallVector<SDValue>` for Div, one for Rem?
> 
> Especially since we're handling 3 different opcodes, possibly 6 in the future for signed types.
Returning two vectors complicates the code in `X86TargetLowering::LowerWin64_i128OP` which would need to check the opcode to know where to get the result since it handles both DIV and REM.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:20492
+    if (expandDIVREMByConstant(N, Result, MVT::i32, DAG))
+        return DAG.getNode(ISD::BUILD_PAIR, SDLoc(N), N->getValueType(0),
+                           Result[0], Result[1]);
----------------
grr why does clang-format keep over indenting this return?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130862