[PATCH] D56214: AggressiveInstCombine: Fold full mul i64 x i64 -> i128

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:267
+
+/// Matches the following pattern producing full multiplication:
+///
----------------
Does it also match the other two I mentioned in the cfe-dev thread? Specifically, where you have (my version `TWO`):

    %u1ls = shl i64 %u1, 32
    %lo = or i64, %u1ls, %t0l

my version `ONE` has:

    %lo = mul i64 %x, %y

and my version `THREE` has:

    %u3 = add i64 %t2, %t1
    %u3ls = shl i64 %u3, 32
    %lo = add i64 %u3ls, %t0

https://godbolt.org/z/_1pDoz


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:361
+                            m_HighPart(m_Specific(y))))) {
+        LLVM_DEBUG(dbgs() << "Hi found!!!\n" << *x << "\n" << *y << "\n");
+
----------------
Remove debugging printf?


================
Comment at: test/Transforms/AggressiveInstCombine/mul128.ll:8
+; CHECK-NOT:   mul nuw i64
+; CHECK:       mul nuw i128
+define { i64, i64 } @mul_full_64(i64 %x, i64 %y) {
----------------
Peanut gallery says: I doubt that this test captures everything that you want to test about the optimization. You just check that the output contains `mul nuw i128`, but what if it contains that instruction plus a bunch more unintended stuff?

But I don't know anything about how LLVM optimizations are usually tested. Maybe this test is fine as-is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56214