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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

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?

> 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?



================
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)));
----------------
Should this be `LL` or `DL`? (I dunno, just asking; curious why `DL` exists).


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7278
+  // Join the remainder halves.
+  SDValue Rem = DAG.getNode(ISD::BUILD_PAIR, dl, VT, RemL, RemH);
+
----------------
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)?


================
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});
+    }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130862