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

Wed Nov 2 05:43:40 PDT 2022

fhahn added a comment.

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

> In D136736#3895386 <https://reviews.llvm.org/D136736#3895386>, @eopXD wrote:
>
>> In D136736#3895242 <https://reviews.llvm.org/D136736#3895242>, @fhahn wrote:
>>
>>> Could you elaborate why a target-specific option is needed? Shouldn't it be beneficial for all targets and be enabled independently?
>>
>> Enabling in all targets will require a lot of changes in test cases across targets so I think I should divide and conquer here.
>
> @fhahn Any more comments before merging this?

Oh I see, there are ~28 LSR tests failing and ~50 codegen tests (with AArch64, ARM & X86 backends).

Are you confident that the patch is working as expected in all cases?

My main worry is that by not updating all tests we may miss bugs in the code. I have no idea if all the assembly changes in the codegen tests for RISCV are beneficial, but I spot-checked the impact on some other tests and both `llvm/test/CodeGen/Thumb2/LowOverheadLoops/vcmp-vpst-combination.ll` and `llvm/test/CodeGen/Thumb2/LowOverheadLoops/tail-pred-intrinsic-fabs.ll` seem to regress with `AllowDropSolutionIfLessProfitable` set to `true. IIUC this might indicate that the cost estimate may not be working as expected in all cases.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136736