[PATCH] D136736: [LSR][TTI][RISCV] Add isAllowDropLSRSolution into TTI and enable it for RISC-V

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Sam Parker implemented a lsr-complexity-limit, which can be set much higher to perform less filtering. The complexity needs to be kept under control though if just for compile time, and it is very easy to go over that with unrolled loops or more complex cases. We should be getting the simple cases correct though, and my view is that we could adjust the filtering to be more profitable. It is just a set of heuristics, after all.

In D136736#3906207 <https://reviews.llvm.org/D136736#3906207>, @eopXD wrote:

> @dmgreen Thank you for giving this a look. My understanding to LSR matches yours. The filtering happens after all interesting variations were generated, and the filtering does not necessarily preserves the original ones, which motivates this patch of considering the LSR derived solution to the original existing one. My main motivation was from `RISCV/lsr-drop-solution.ll`, which the original LSR suggestion does make a worse transformation than the original unchanged IR. This patch is an amendment that comes from the fallacy in filtering.

I see some intrinsics that I guess are loads and stores. Have you thought about implementing getTgtMemIntrinsic for them? I'm not sure it will help, but the example doesn't look very complex. If it is filtering the wrong solutions I would look into why and if it is possible to prevent it. After all, we could always start with the "bad" solution and then never see the "good" alternative because of it.  I'm not really against this patch though, so long as it doesn't get enabled for targets where it is obviously worse. It would be better if the "cost" could be relied upon but that doesn't seem to currently be possible in all cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136736