[PATCH] D50310: Improve the legalisation lowering of UMULO

[Simonas Kazlauskas via Phabricator via llvm-commits]

nagisa added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2743
+    // know how to expand `i64,i64 = umul_lohi a, b` and abort (why isn’t this
+    // operation recursively legalized?).
+    //
----------------
efriedma wrote:
> Transforming umul_lohi to a call to __multi3 isn't particularly useful. We could expand it inline, but it's a lot of code.
The intention to use `umul_lohi` was to specifically give targets the information that a widening multiply is expected here, so targets which natively support this operation could do that without necessarily inspecting the operands for `ZERO_EXTEND`.

Alas, targets like 32-bit ARM outright refuse to lower such operation for `i64,i64` e.g. output, and, I assume, many more targets would have trouble with `i128,i128 umul_lohi`.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2717
+    // %3 = mul nuw iN (%LHS.LOW as iN), (%RHS.LOW as iN)
+    // %4 = add iN (%1.0 as iN) << Nh, (%2.0 as iN) << Nh
+    // %5 = { iN, i1 } @uadd.with.overflow.iN( %4, %3 )
----------------
efriedma wrote:
> This add can overflow, but I guess that can only happen if %0 is true?
When this operation overflows, one or more of the `%1.1`, `%2.1` and `%0` will already be true and thus the whole operation will already have overflow bit set.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2754
+    SDValue OneInHigh = DAG.getNode(ISD::SHL, dl, VT,
+      DAG.getNode(ISD::ANY_EXTEND, dl, VT, One.getValue(0)), ShiftAmount);
+    SDValue TwoInHigh = DAG.getNode(ISD::SHL, dl, VT,
----------------
efriedma wrote:
> Maybe BUILD_PAIR here instead, since it's going to get split anyway?
Will try, but not sure if you can add two "pairs" together.


https://reviews.llvm.org/D50310