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

Thu May 6 22:34:05 PDT 2021

dvyukov added a comment.

In D101536#2743055 <https://reviews.llvm.org/D101536#2743055>, @lebedev.ri wrote:

> In 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?

All benchmarks are noisy.
I can only recommend gentleman's set of disabling CPU autoscaling, killing all background processes, pinning the test process to a single core, taking minimum run time of multiple runs (no affected by random slowdowns).

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

I've seen this effect a lot. You are not losing your mind :)
But I've seen it withing +/-3% on Intel CPUs. But maybe that's combined with the noise you mentioned before.

> In 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 LLVM Github Monorepo

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

https://reviews.llvm.org/D101536