[PATCH] D41766: [MachineCombiner] Add check for optimal pattern order.

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso accepted this revision.
mssimpso added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D41766#972219, @fhahn wrote:

> TBH I was hoping for a more complete approach to testing this, but I thought I share this relatively straight forward check.


I'm fine with that. We can always improve it later.

> I've run the LLVM test suite with this patch (without the EXPENSIVE_CHECKS guard) on AArch64 with `-O3 -ffast-math` and various mcpu options. I also run the patch on a single X86 machine with -mcpu=native. Beyond that, I did not run this patch on any other targets (except all LLVM unit tests with this change)

OK, sounds good. I'm just trying to avoid the case where some in-tree target is intentionally adding the patterns in a way that doesn't increase expected latency. Though if that's the case, they might want to know.

This seems useful enough to me to commit.



================
Comment at: test/CodeGen/AArch64/aarch64-combine-fmul-fsub.mir:29-30
 # PROFITABLE-LABEL: name: f1_2s
-# PROFITABLE: %5:fpr64 = FNEGv2f32 %2
-# PROFITABLE-NEXT: FMLAv2f32 killed %5, %0, %1
+# PROFITABLE: [[R1:%[0-9]+]]:fpr64 = FNEGv2f32 %2
+# PROFITABLE-NEXT: FMLAv2f32 killed [[R1]], %0, %1
 ---
----------------
fhahn wrote:
> mssimpso wrote:
> > These tests don't need changing because of this patch, right? But you can go ahead and commit the test changes separately if you need to.
> Unfortunately they do. Some patterns, at least on AArch64 add additional virtual registers when the pattern is instantiated. Unused virtual registers should be dropped by later passes, but they change the numbers of the registers in this test case.
Ahh, that makes sense. Thanks!


https://reviews.llvm.org/D41766