[LSV] Load Store Vectorizer failed to combine two related loads.

[cdevadas]

The LSV pass doesn't combine the loads even though the base address remain the same for them.
There are many instances found in the AMDGPU codegen lit test folder. For example, llvm/test/CodeGen/AMDGPU/GlobalISel/fp-atomics-gfx940.ll (in function local_atomic_fadd_v2bf16_noret) should have the two loads combined earlier. But they are merged using the target specific Load Store Optimizer pass (si-load-store-opt) after ISel.

[jayfoad]

llvm/test/CodeGen/AMDGPU/GlobalISel/fp-atomics-gfx940.ll (in function local_atomic_fadd_v2bf16_noret)

I don't see that function in that file. Can you provide a link?

[cdevadas]

Looks like I have a slightly older version of upstream compiler. This test has been changed. However the same test compiled for SelectionDAG still exists and it reproduces the problem.
https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AMDGPU/fp-atomics-gfx940.ll#L364

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: Christudasan Devadasan (cdevadas)

The LSV pass doesn't combine the loads even though the base address remain the same for them. There are many instances found in the AMDGPU codegen lit test folder. For example, llvm/test/CodeGen/AMDGPU/GlobalISel/fp-atomics-gfx940.ll (in function local_atomic_fadd_v2bf16_noret) should have the two loads combined earlier. But they are merged using the target specific Load Store Optimizer pass (si-load-store-opt) after ISel.

[arsenm]

I think this is just from unhandled insertion of bitcasts to get the types to match for vectorization:

; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx940 -print-after=load-store-vectorizer < %s

define amdgpu_kernel void @no_vectorize_0(ptr addrspace(3) %ptr, <2 x half> %data) {
  %i1 = atomicrmw fadd ptr addrspace(3) %ptr, <2 x half> %data syncscope("agent") seq_cst, align 4
  ret void
}

define amdgpu_kernel void @no_vectorize_1(i32 %ptr.as.int, <2 x half> %data) {
  %ptr = inttoptr i32 %ptr.as.int to ptr addrspace(3)
  %i1 = atomicrmw fadd ptr addrspace(3) %ptr, <2 x half> %data syncscope("agent") seq_cst, align 4
  ret void
}

define amdgpu_kernel void @does_vectorize(i32 %ptr.as.int, i32 %data.as.int) {
  %ptr = inttoptr i32 %ptr.as.int to ptr addrspace(3)
  %data = bitcast i32 %data.as.int to <2 x half>
  %i1 = atomicrmw fadd ptr addrspace(3) %ptr, <2 x half> %data syncscope("agent") seq_cst, align 4
  ret void
}

[gandhi56]

This issue appears to be resolved for the above test and @flat_atomic_fadd_v2bf16_noret from llvm/test/CodeGen/AMDGPU/GlobalISel/fp-atomics-gfx940.ll. Should we close this issue, @arsenm @cdevadas ?

[arsenm]

This issue appears to be resolved for the above test and @flat_atomic_fadd_v2bf16_noret from llvm/test/CodeGen/AMDGPU/GlobalISel/fp-atomics-gfx940.ll. Should we close this issue, @arsenm @cdevadas ?

No, this is not fixed at all. The specific function instance doesn't matter. You can see this is any kernel signature with mixed types. See the output of my last comment. The first 2 functions are more representative cases, and the last kernel is the manually coerced version. The vectorizer fails to pack the first 2, and the MIR pass has to fix up the missed vectorization

[gandhi56]

It seems that vectorization only occurs when the type of %data is i32 after removing all bitcasts.

[arsenm]

It seems that vectorization only occurs when the type of %data is i32 after removing all bitcasts.

Yes, it's easiest to handle a bunch of uniformly typed arguments. The weird mismatched vectors and scalars aren't handled

[gandhi56]

define amdgpu_kernel void @no_vectorize_0(ptr addrspace(3) %ptr, <2 x half> %data) {
  %no_vectorize_0.kernarg.segment = call nonnull align 16 dereferenceable(264) ptr addrspace(4) @llvm.amdgcn.kernarg.segment.ptr()

  ; load pointer
  %ptr.load = load ptr addrspace(3), ptr addrspace(4) %no_vectorize_0.kernarg.segment, align 16

  ; load data
  %data.kernarg.offset = getelementptr inbounds i8, ptr addrspace(4) %no_vectorize_0.kernarg.segment, i64 4
  %data.load = load <2 x half>, ptr addrspace(4) %data.kernarg.offset, align 4

  %i1 = atomicrmw fadd ptr addrspace(3) %ptr.load, <2 x half> %data.load syncscope("agent") seq_cst, align 4
  ret void
}

In this example, %ptr.load can be replaced by a load i32 and an inttoptr, and %data.load can be replaced by a load i32 and a bitcast i32 to <2 x half>. This way, LSV should be able to vectorize the two loads into a load <2 x i32>. This seems scalable for all allowed types for atomicrmw fadd.