[PATCH] D50310: Improve the legalisation lowering of UMULO

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

> I would very much love to add a test that actually executes some machine code with a number of test vectors, but I believe no such test family exists in LLVM.

That would require being able to execute code for an arbitrary target, which would in general require an emulator, which is a lot more hassle than it's worth.

-----

No tests for umul.with.overflow.i64 on 32-bit?



================
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?).
+    //
----------------
Transforming umul_lohi to a call to __multi3 isn't particularly useful. We could expand it inline, but it's a lot of code.


================
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 )
----------------
This add can overflow, but I guess that can only happen if %0 is true?


================
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,
----------------
Maybe BUILD_PAIR here instead, since it's going to get split anyway?


================
Comment at: test/CodeGen/ARM/muloti2.ll:3
+; RUN: llc < %s -mtriple=armv6-unknown-linux-gnu | FileCheck %s --check-prefixes=ARMV6
+; RUN: llc < %s -mtriple=armv7-unknown-linux-gnu | FileCheck %s --check-prefixes=ARMV7
+; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu | FileCheck %s --check-prefixes=AARCH
----------------
v6 and v7 are basically identical for this purpose; the code isn't using any v7-specific instructions.


================
Comment at: test/CodeGen/ARM/muloti2.ll:4
+; RUN: llc < %s -mtriple=armv7-unknown-linux-gnu | FileCheck %s --check-prefixes=ARMV7
+; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu | FileCheck %s --check-prefixes=AARCH
+
----------------
AArch64 tests aren't allowed in test/CodeGen/ARM; they have to go in test/CodeGen/AArch64.  (It's possible to build LLVM with the AArch64 backend enabled, but not the ARM backend.)

Probably should also test ARMv6 in Thumb mode for completeness (although I expect the result to be messy).


https://reviews.llvm.org/D50310