[PATCH] D140542: [MachineCombiner] Support local strategy for traces

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D140542#4086019 <https://reviews.llvm.org/D140542#4086019>, @asi-sc wrote:

> Thanks for the suggestion, I agree it can fix the provided test, but it won't solve the problem in general: when machine combiner estimates profitability, it bases the decision on the trace gathered according to some strategy. The current implementation supports only one strategy that choses multiple blocks from a function. So, when we ask machine combiner to check whether the suggested pattern is good or bad, an instruction that was really far away (let's say 4 BBs and 400 instructions above) may dramatically affect its decision (when it is on the critical path). It works fine for CPUs with big OOO buffer, but not for a tiny CPU that executes instructions in-order. My experiments show that for such CPUs we should calculate critical path separately for each BB as instructions that were in other BBs usually has almost no effect. Does it sound reasonable?

Yes, that seems like a valid approach. However, I have no experience with any recent in-order cores, so it would be interesting to see if the experimental results that you got with sifive-u74 can be replicated for other in-order cores.

A quick scan of in-tree targets with `MicroOpBufferSize = 0` says that we could try benchmarking on SiFive7 and a variety of Arm CPUs (M4, M55, Cortex-M7, Cortex-A55, etc).

IIUC, only RISCV is overriding the default with this patch, so no other arch will be affected, and so I don't think we need to hold this patch up waiting for more experimental data. 
But does that mean the test difference that you are showing will also occur for SiFive7 with this patch? If so, can you add a RUN line to the test file like that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140542