Typo in summary: successfor -> successor ?

This doesn't fix the problem that inspired D71717. Consider the first test case in memory_clause.ll. With baseline llvm I get:

$ bin/llc -march=amdgcn -mcpu=gfx902 -verify-machineinstrs -amdgpu-enable-global-sgpr-addr -o /dev/null ~/git/llvm-project/llvm/test/CodeGen/AMDGPU/memory_clause.ll -debug-only=machine-scheduler |& egrep "^Cluster|Machine code for function"
# Machine code for function vector_clause: NoPHIs, TracksLiveness
Cluster ld/st SU(2) - SU(3)
Cluster ld/st SU(6) - SU(7)
Cluster ld/st SU(8) - SU(9)
Cluster ld/st SU(11) - SU(13)
# Machine code for function vector_clause: NoPHIs, TracksLiveness
[...]

Your patch doesn't change this, but with D71717 I get:

$ bin/llc -march=amdgcn -mcpu=gfx902 -verify-machineinstrs -amdgpu-enable-global-sgpr-addr -o /dev/null ~/git/llvm-project/llvm/test/CodeGen/AMDGPU/memory_clause.ll -debug-only=machine-scheduler |& egrep "^Cluster|Machine code for function"
# Machine code for function vector_clause: NoPHIs, TracksLiveness
Cluster ld/st SU(2) - SU(3)
Cluster ld/st SU(6) - SU(7)
Cluster ld/st SU(8) - SU(9)
Cluster ld/st SU(10) - SU(11)
Cluster ld/st SU(12) - SU(13)
# Machine code for function vector_clause: NoPHIs, TracksLiveness
[...]

So now it is considering all of SU(10) .. SU(13) for clustering, instead of just SU(11) and SU(13). The relevant SUs are:

SU(6):   %12:vreg_128 = GLOBAL_LOAD_DWORDX4_SADDR %26:vreg_64, %4:sreg_64_xexec, 0, 0, 0, 0, implicit $exec, implicit $exec :: (load 16 from %ir.tmp3, addrspace 1)
SU(7):   %15:vreg_128 = GLOBAL_LOAD_DWORDX4_SADDR %26:vreg_64, %4:sreg_64_xexec, 16, 0, 0, 0, implicit $exec, implicit $exec :: (load 16 from %ir.tmp72, addrspace 1)
SU(8):   %17:vreg_128 = GLOBAL_LOAD_DWORDX4_SADDR %26:vreg_64, %4:sreg_64_xexec, 32, 0, 0, 0, implicit $exec, implicit $exec :: (load 16 from %ir.tmp116, addrspace 1)
SU(9):   %19:vreg_128 = GLOBAL_LOAD_DWORDX4_SADDR %26:vreg_64, %4:sreg_64_xexec, 48, 0, 0, 0, implicit $exec, implicit $exec :: (load 16 from %ir.tmp1510, addrspace 1)
SU(10):   GLOBAL_STORE_DWORDX4_SADDR %26:vreg_64, %12:vreg_128, %5:sreg_64_xexec, 0, 0, 0, 0, implicit $exec, implicit $exec :: (store 16 into %ir.tmp5, addrspace 1)
SU(11):   GLOBAL_STORE_DWORDX4_SADDR %26:vreg_64, %15:vreg_128, %5:sreg_64_xexec, 16, 0, 0, 0, implicit $exec, implicit $exec :: (store 16 into %ir.tmp94, addrspace 1)
SU(12):   GLOBAL_STORE_DWORDX4_SADDR %26:vreg_64, %17:vreg_128, %5:sreg_64_xexec, 32, 0, 0, 0, implicit $exec, implicit $exec :: (store 16 into %ir.tmp138, addrspace 1)
SU(13):   GLOBAL_STORE_DWORDX4_SADDR %26:vreg_64, %19:vreg_128, %5:sreg_64_xexec, 48, 0, 0, 0, implicit $exec, implicit $exec :: (store 16 into %ir.tmp1712, addrspace 1)

Add a TODO comment about M0.

Use CHECK-NEXT instead of CHECK-NOT.

Ping!

Can you add G_READCYCLECOUNTER to docs/GlobalISel/GenericOpcode.rst?

Can you add G_READ_REGISTER and G_WRITE_REGISTER to docs/GlobalISel/GenericOpcode.rst?

Can you add G_READ_REGISTER and G_WRITE_REGISTER to docs/GlobalISel/GenericOpcode.rst?

Track liveness.

LGTM! Perhaps add a comment in TargetInstrInfo.h documenting the argument? E.g. "the number of loads that will be in the cluster if this hook returns true"...?

I just cannot deal with any single performance regression in all of the targets, including out of tree targets.

In D73292#1837231, @rampitec wrote:

In D73292#1837215, @foad wrote:

I tried something similar in D72325.

Comments there argue about how much should we cluster, but regardless I do not think we should use a wrong data. If we want more clustering we need to increase thresholds, but still rely on a correct input.

I tried something similar in D72325.

Rebase and address some review comments.

Add legalize tests for signed 16-bit operations.

Ping!

Use MIRBuilder.getTII().

Remove G_SMULO/G_UMULO changes which have been split out into D72842.

Sorry @arsenm, I updated the patch just before I saw your approval.

Added similar fixes in IRTranslator.cpp.

This patch would only change behaviour if the target's shouldClusterMemOps(SUa, SUb) might return a different answer from shouldClusterMemOps(SUb, SUa).

In D72737#1821937, @rampitec wrote:

We have moved uses of loaded values further from their loads, which is good. As far as I understand these changes are inducted by the removal of artificial edges which were created by MemOpClusterMutation. These edges were linking successors of any load to all the nodes in a cluster and restricted the scheduling.
In sign_extend.ll that is because of the store clustering, we have moved v_ashrrev_i32_e32 producing v2 past v_ashrrev_i32_e32 producing v3 because store cluster uses them in this order. Before it was harder to do because of the artificial edges linking all predecessors to all stores.

Rebase.

There are nice changes in a bunch of tests, where we're preserving clusters instead of breaking them apart.

LGTM

In D65088#1817656, @arsenm wrote:

Any update on this?

Here's the disassembler diff with a bit more context:

diff --git a/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt b/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt
index c1ec51ee7ad..3e040460988 100644
--- a/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt
+++ b/llvm/test/MC/Disassembler/AMDGPU/gfx10_dasm_all.txt
@@ -5773,17 +5773,14 @@

I'd prefer to call adjustSchedDependency unconditionally, but we can do that later.

New tests for this specific fix:
test/CodeGen/PowerPC/botheightreduce.mir
test/CodeGen/PowerPC/topdepthreduce-postra.mir
test/CodeGen/X86/topdepthreduce-postra.mir

This is only relevant for post-ra scheduling because we don't have any bundles when we do pre-ra scheduling, right?

In D72325#1811961, @nhaehnle wrote:

Don't we *want* clusters that large, and even larger?

In D72392#1811691, @fhahn wrote:

Also, do you have any performance/codesize numbers for impacted targets?

The diff in llvm/test/CodeGen/X86/testb-je-fusion.ll is:

        movl    %edi, %eax
-       addl    $-512, %eax             # imm = 0xFE00
        movb    $1, (%rsi)
+       addl    $-512, %eax             # imm = 0xFE00
        je      .LBB2_2
...
        movl    %edi, %eax
-       decl    %eax
        movb    $1, (%rsi)
+       decl    %eax
        je      .LBB3_2

The scheduler prefers not to put the addl/decl immediately after the first movl because of the register dependency on eax with latency 1.

The diff in llvm/test/CodeGen/AArch64/arm64-zero-cycle-zeroing.ll is:

        adrp    x8, .LCPI0_0
-       ldr     h0, [x8, :lo12:.LCPI0_0]
        movi    v1.2d, #0000000000000000
        movi    v2.2d, #0000000000000000
+       ldr     h0, [x8, :lo12:.LCPI0_0]
        movi    v3.2d, #0000000000000000

The scheduler prefers not to put the ldr immediately after the adrp because of the register dependency on x8 with latency 1.

In D71655#1810177, @arsenm wrote:

Mostly LGTM, except this diff seems to have absorbed a number of recent changes into the diff

Adding target owners to review the (mostly mechanical) changes to affected targets.

New simpler approach. This version only introduces one new target hook,
getMemOperandsWithOffset, and leaves the old getMemOperandWithOffset as
a convenience function. It also leaves the immediate Offset as an
int64_t value in bytes, instead of trying to represent it as one or more
MachineOperands.