Sinking load instructions results in worse performance and increased dynamic instruction counts

[kazutakahirata]

SimplifyCFG recently gained ede27d8 by @nikic. Now, it seems to backfire in some cases:

Compile attached bcmp.ll (generated from llvm-project/libc/src/string/bcmp.cpp) like so:

$ clang -O3 -S bcmp.ll -o bcmp.s

Then I get:

Without the patch:

# %bb.26:
        movdqu  -16(%rdi,%rdx), %xmm0
        movdqu  -16(%rsi,%rdx), %xmm1
        jmp     .LBB1_34
:
:
:
# %bb.33:
        movdqu  (%rdi,%rdx), %xmm0
        movdqu  (%rsi,%rdx), %xmm1
.LBB1_34:                               # %.loopexit

With the patch:

# %bb.25:
        addq    %rdx, %rdi
        addq    $-16, %rdi
        addq    %rdx, %rsi
        addq    $-16, %rsi
        jmp     .LBB1_33
:
:
:
# %bb.32:
        addq    %rdx, %rdi
        addq    %rdx, %rsi
.LBB1_33:                               # %.loopexit.sink.split
        movdqu  (%rdi), %xmm0
        movdqu  (%rsi), %xmm1

Notice that the two load instructions sink just below the join point while the address calculation is left behind. This seems to result in worse performance and increased dynamic instruction counts.

[nikic]

Yeah, splitting apart GEP + load may be non-profitable, if it was previously a legal addressing mode. I wonder whether we should adjust sinking to not do this, or undo it in CGP -- the latter already does a lot of the "try to form legal addressing modes" optimizations.

[bgra8]

@nikic The mentioned culprit ede27d8 introduces ~20% performance regressions for bcmp libc functions on Intel architectures (Skylake especially). The regressions can be measured using the libc benchmark in llvm-project/libc/benchmarks/LibcMemoryGoogleBenchmarkMain.cpp.

Can you consider reverting based on this?

[david-xl]

Before we find a real solution, we should probably revert this one or put the change under a flag.

[nikic]

The change also fixes multiple other 20% regressions (see #94092 for the original context) and is an improvement on average. As such, I don't think reverting it makes sense, as we'd just trade one set of regressions for another, while regressing average performance and code size.

[david-xl]

Or improve the patch with suggestion 1 (not do sinking if it split GEP + LOAD which are legal address mode) for now? Figuring out the right place to fix can take some time.

[perlfu]

Out of curiosity, is anyone actively working on this?

[piotrAMD]

If this issue is not being actively worked on I agree with @bgra8 and @david-xl that the offending commit should either be reverted or worked around.

For the record, we are seeing ~11% drop in one benchmark on AMDGPU.