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

[Paweł Bylica via Phabricator via llvm-commits]

chfast marked 3 inline comments as done.
chfast added inline comments.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:267
+
+/// Matches the following pattern producing full multiplication:
+///
----------------
Quuxplusone wrote:
> 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
As mentioned before, currently the optimization matches patterns for low and high independently. Mostly, because I don't know yet what is the best way to combine both.

Currently for low it replaces pattern `TWO` with `ONE`. The `THREE` will not work.

These are great test, will add them to the test suite in a separate review as suggested.


================
Comment at: lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:297
+
+  Value *x = nullptr;
+  Value *y = nullptr;
----------------
craig.topper wrote:
> Don't you need to check the type is i64 somewhere? Or did I miss it?
Yes, I should. Is there a way to do this check with `match()`. I have not found any example doing this.


================
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) {
----------------
Quuxplusone wrote:
> 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.
@lebedev.ri already suggested how to generate better checks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56214