LGTM

Similar fixes review: D141848
LLVM YAML parser improvement that depends on this: D140474

In D140542#4096793, @spatel wrote:

The asm diffs in existing test files show that we are affecting the default behavior of RISC-V compiles, right? If those are considered neutral or improvements, then I think this is good to go.

Use MinInstrCount strategy when scheduling model is not specified.

@yota9 , @cishida , @spowell , @steven_wu , sorry for mentioning you directly in this review, however I believe you are the right experts to consult about the content of some tests. We need to get rid of keys duplication in YAML inputs as keys duplication is not allowed by the specification. These fixes in tests blocks improvement of LLVM's YAML parser. Could you please check the comments below where I've mentioned you?

Rebase: llvm/test/tools/llvm-objcopy/ELF/strip-all-gnu.test was fixed in D143086

Discussion related to this fix can be found in D141848 (see 1 , 2, 3).

In D140542#4090627, @spatel wrote:

But does that mean the test difference that you are showing will also occur for SiFive7, Syntacore SCR1, and/or Rocket with this patch? If so, can you add a RUN line to the test file like that?

Improve test coverage

@jhenderson , thanks for your investigation and really detailed explanation!

Address review comments

In D140542#4083878, @spatel wrote:

I don't see anything wrong with this patch, but I haven't looked at MachineCombiner in a long time, so I'm likely not the best reviewer.

I do want to note a potential alternative - have you looked at using enableAggressiveFMAFusion() in DAGCombiner?
If you flip that switch for RISCV, the sub3 calculation in the last block would become:

	fmadd.d	ft1, ft0, fa1, fa0
	fnmsub.d	fa0, ft0, fa1, ft1

If the fma instruction timing is the same as plain fmul on your target, then is that even better than the 1 fmadd produced on the test here?

Ping

Ping

Gentle ping

Adding to review test creators as some changes are not trivial.

Remove tests in yaml2obj, add unittest for YAMLIO

Remove one incorreclty added test

Move test changes to a separate review D141848

For in-order cores MachineCombiner makes better decisions when the critical path

@asi-sc did you do any measurements to collect empirical data?

Ping

Ping

Gentle ping

Rebase

Rebase

Rebase

Rebase

Previously, we unconditionally applied the first legal pattern - without any consideration of profitability

Rebase

LGTM too.

Thanks, no more comments from my side.

Have you had a chance to make some performance measurements?

Update RISCV/machine-combiner.ll

Sync with changes in parent patches

Sync with changes in parent patches

Forgot to change enum name in MachineCombiner. Fixed

Fix BOLT AArch64/plt-gnu-ld.test

Clang-format .cpp tests

A gentle ping.

Since we've moved RISCV reassociation implementation to generic machine combiner mechanism D138302 and added reassociation of inverse instructions to machine combiner in D136754, we now can enable reassociation of inverse for RISCV.

Add tests to cover fadd/fsub reassociation; rebase

Thanks for your feedback, Philip. I'll made some experiments with this change at my free time, so not abandoning it right now. However, I mark it as "plan changes" to remove it from review queues.

Address review comments

sync changes with parent revision, rebase

Change Optional to std::optional, rebase

@reames , could you please take a look at my previous comment? If you still think we should proceed with the minimized version you made, will you drive the review process? I don't see any progress there for three weeks.