[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.

MVE relies pretty heavily on AMK_PostIndexed from getPreferredAddressingMode. I could easily imagine something could be going wrong there. But I don't see this as profitable for Thumb1 targets or AArch64 either, when ran across a wide set of benchmarks. We certainly know that LSR is important for performance, and there are a number of known issues where it is not optimal.

This is my brief understanding of how LSR works:

- Create a bunch of little formula that look useful (isLegalUse) and can be combined in different ways for loop varying accesses.
- Realise that there are far too many formula combinations and start filtering them out based on heuristics of which ones look unprofitable.
- "Solve" by considering all the remaining formula for which gives the best cost.
- Use this best solution.

The creation and filtering isn't unimportant for profitability. The algorithm as a whole is presumably assuming that certain formula are already ruled out when it comes to costing them, at least undef MVE. Even if they are the existing formula.

The best cost is by default based on the Number of Registers used, followed by AddRecCost, Muls, Adds, Imms and SetupCost. X86 and some other architectures also consider Number of Instructions.  The NumRegs often isn't the most important issue in a loop. In the past I had tried altering the cost on Arm/AArch64 to not consider NumRegs as the most important factor, but the performance wasn't good enough to justify it.  LSR is pretty fragmented already though between different backends as a result.

Ideally LSR shouldn't need patched like this, even if I'm not against it for other architectures. It sounds like it should be sensible enough. But why isn't the most profitable formula already being considered in the Solve? Is it never generated, or is it filtered out? I don't know RISCV assembly very well. A lot of the examples look like they have more instructions in the loop to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136736