[PATCH] D101536: [SimplifyCFG] Common code sinking: drop profitability check in presence of conditional predecessors (PR30244)

Thu May 6 15:24:36 PDT 2021

xbolva00 added a comment.

Offline question - Would you like to try spec2006/2017 for better benchmarking?

I can provide it you, privately, with no futher sharing promise.

> Dňa 7. 5. 2021 o 0:07 užívateľ Roman Lebedev via Phabricator <reviews at reviews.llvm.org> napísal:
>
> lebedev.ri added a comment.
>
> In D101536#2742629 <https://reviews.llvm.org/D101536#2742629> https://reviews.llvm.org/D101536#2742629, @xbolva00 wrote:
>
>> Can you collect some performance stats?
>>
>> - testsuite/spec benchmarks/etc?
>>
>>>> Speculatively land and see if some other perf regression is reported against this change?
>>
>> Try to collect some data first
>
> Thank you for stating your opinion.
> As you can read in my previous comments, i have obviously already tried that locally.
> test-suite is just hopelessly noisy, does that match with your expirience?
>
>> On the benchmark this does affect, the results are bizzare to me,
>> because there are *no* IR, or ASM level changes on the codepath being run,
>> yet there's +~10% run-time regression. Unless i'm slowly (rapidly) loosing it,
>> i can only attribute it to some weird code alignment issue
>> caused by the changes this did cause to other code.
>>
>> In D101536#2742764 <https://reviews.llvm.org/D101536#2742764> https://reviews.llvm.org/D101536#2742764, @davidxl wrote:
>>
>> please add more test cases to cover the expected new sinking cases enabled with this change.
>
> The test covers it quite well - if we'll only sink already-speculatable instructions,
> and not all predecessors are unconditional, we will now actually sink it.
> So could you please be more specific what tests would you like to see?
>
> Repository:
>  rG <https://reviews.llvm.org/source/llvm-github/> LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D101536/new/
>
> https://reviews.llvm.org/D101536

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101536