[AMDGPU][CodeGen] Match SMRDs with constant bases and register offsets.

[kosarev]

Summary

Saves some add instructions on a couple Rage 2 shaders and is also a
prerequisite for a coming-soon change matching (register + immediate)
offsets.

[kosarev]

D128836 covers the case for Global ISel.

[foad]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

36

When this patch and D128836 have both landed, can you add GISEL checks here please.

38

Please don't use unnamed values in tests. (You can run the IR through opt -instnamer to name them automatically.)

40

Do I understand correctly: in SDag this gep ends up as (add %2, @0) (not (add @0, %2)) because @0 is a constant so it gets canonicalized to the RHS?

[kosarev]

Rebased on top of D128171 and updated the test to use named values.

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

36

Done; the GCN checks now work for both the SDAG and GISEL runs.

40

That's right, for commutative nodes the combiner tries to make whatever seems a constant be the second operand.

[foad]

LGTM, thanks!

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

1–3

Should use explicit global-isel=0 for dag test

34–35

Should test the operands

45

Doesn't test the 32-bit constant case

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

Why would we want to check that? Looks like there's nothing specific to 32-bit constants in the change?

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

You have the call to Expand32BitAddress

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

Yes, which replicates the existing and already-tested logic. Will it eliminate the need for the test if I move the couples of SelectSMRDOffset() and Expand32BitAddress() calls into a separate function, something like SelectSMRDBaseOffset()? We would need it for matching (register + immediate) offsets anyway.

Feels like that'd be a test case for what is actually not a special case.

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

It's another permutation of a somewhat fragile case (which also works differently in globalisel). There's no reason to skimp on tests

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

The if ((Addr.getValueType() != MVT::i32 || Addr->getFlags().hasNoUnsignedWrap())) bit in AMDGPUDAGToDAGISel::SelectSMRD() means for such a test we would need the nuw flag be set on the ISD::ADD in the offset, which we only do for constant getelementptr inbounds indexes that in turn would always go to the RHS operand of the ISD::ADD, and thus can't help reproducing the case.

D15544 gives some reasoning for raising nuw for non-negative constant offsets. If we think it's worth it, I could try to do the same for non-constants known to be small enough, but it again would help if we do not require cultivating things specific to the constant address space be a prerequisite for this generic change.

[kosarev]

Addressed review notes.

[kosarev]

Update the GISel test for it to be less dependent on code order.

[foad]

Match complex register SMRD offsets.

Could you improve the commit message? I don't know what "complex" means here. I think it is really about matching (constant base + register offset) in addition to (register base + constant offset).

[kosarev]

Reworded commit message.

[kosarev]

Right, the 'complex' part came from when I had a hypothesis that it is the size of the operand that matters. Thanks for catching.

[foad]

Still LGTM if Matt is happy.

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

You can just add noinbounds to the getelementptr. It's not a special change and just testing the mechanics for what you already have here

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

Sorry, I'm not sure I follow. What is noinbounds and how it should be used with the getelementptr, exactly?

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

@arsenm Ping?

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

You can try this:

%i3 = getelementptr inbounds [4 x <2 x float>],

with the addrspace(6) pointer

[kosarev]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

It has already been explained in https://reviews.llvm.org/D129095#inline-1241075 that that wouldn't work.

On top of that, we don't seem to support addrspace(6) constants in SDAG ISel currently; I get AMDGPU DAG->DAG Pattern Instruction Selection error whenever I try to use one.

Regarding noinbounds, I don't see how your words explain what it is or how it can be useful for triggering the added code. Is that your comment still relevant?

And just for the record, I don't understand the argument about the new Expand32BitAddress() call being a sufficient reason for another test case. We usually don't add tests for higher level code just because it employs more of some lower level functions. It's up to tests specifically for these lower level functions to make sure they can handle their intended uses cases, and as hopefully is obvious from https://reviews.llvm.org/D129381 that eliminates multiple calls to Expand32BitAddress(), this patch doesn't add any new use cases for that function.

What has been changed is that we now support another permutation of operands in SelectSMRD(), but that is already covered with the added tests. addrspace(6) operands are no special in this regard, so adding more tests for them would mean we try to test combinations that are not special implementation-wise, which is again not how we write regression tests. And yes, unnecessary tests can actually be harmful -- they draw maintenance resources and attention and they hide cases that are really important. I believe some of our MC tests illustrate that.

Overall, I feel somewhat confused about your two last comments. If you can reword what you have to suggest in a bit more explicit way, that will likely be a great help. Thanks.

[kosarev]

Updated a comment in the .ll test to not mention complex operands.

[arsenm]

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

45

The point isn't to test Expand32BitAddress but to test that it's used on this path. I guess if constants don't work for constant 32-bit that's a separate issue

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

if ((Addr.getValueType() != MVT::i32 ||
     } else if (getBaseWithOffsetUsingSplitOR(*CurDAG, Addr, N0, N1)) {
       assert(N0 && N1 && isa<ConstantSDNode>(N1));
     }
     if (N0 && N1) {
       if (SelectSMRDOffset(N1, Offset, Imm, Imm32Only)) {
         SBase = Expand32BitAddress(N0);
         return true;
       }
+      if (SelectSMRDOffset(N0, Offset, Imm, Imm32Only)) {
+        SBase = Expand32BitAddress(N1);
+        return true;
+      }
     }
     return false;
   }
   if (!Imm)
     return false;
   SBase = Expand32BitAddress(Addr);
   Offset = CurDAG->getTargetConstant(0, SL, MVT::i32);
   return true;

llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll

-; Test that DAG->DAG ISel is able to pick up the S_LOAD_DWORDX4_SGPR instruction that fetches the offset
-; from a register.
-
-; RUN: llc -march=amdgcn -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefix=GCN %s
-; RUN: llc -march=amdgcn -global-isel -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefix=GISEL %s
+; RUN: llc -march=amdgcn -global-isel=0 -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefixes=GCN,SDAG %s
+; RUN: llc -march=amdgcn -global-isel=1 -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefixes=GCN,GISEL %s
 
-; GCN: %[[OFFSET:[0-9]+]]:sreg_32 = S_MOV_B32 target-flags(amdgpu-abs32-lo) @DescriptorBuffer
-; GCN: %{{[0-9]+}}:sgpr_128 = S_LOAD_DWORDX4_SGPR killed %{{[0-9]+}}, killed %[[OFFSET]], 0 :: (invariant load (s128) from %ir.13, addrspace 4)
+@0 = external dso_local addrspace(4) constant [4 x <2 x float>]
+@1 = external dso_local addrspace(4) constant i32
 
+; Test that DAG->DAG ISel is able to pick up the S_LOAD_DWORDX4_SGPR instruction that fetches the offset
+; from a register.
+; GCN-LABEL: name: test_load_zext
+; SDAG: %[[OFFSET:[0-9]+]]:sreg_32 = S_MOV_B32 target-flags(amdgpu-abs32-lo) @DescriptorBuffer
+; SDAG: %{{[0-9]+}}:sgpr_128 = S_LOAD_DWORDX4_SGPR killed %{{[0-9]+}}, killed %[[OFFSET]], 0 :: (invariant load (s128) from %ir.13, addrspace 4)
 ; GISEL: $[[OFFSET:.*]] = S_MOV_B32 target-flags(amdgpu-abs32-lo) @DescriptorBuffer
 ; GISEL: S_LOAD_DWORDX4_SGPR killed renamable {{.*}}, killed renamable $[[OFFSET]], 0 :: (invariant load (<4 x s32>) from {{.*}}, addrspace 4)
-
 define amdgpu_cs void @test_load_zext(i32 inreg %0, i32 inreg %1, i32 inreg %resNode0, i32 inreg %resNode1, <3 x i32> inreg %2, i32 inreg %3, <3 x i32> %4) local_unnamed_addr #2 {
 .entry:
   %5 = call i64 @llvm.amdgcn.s.getpc() #3
   %6 = bitcast i64 %5 to <2 x i32>
   %7 = insertelement <2 x i32> %6, i32 %resNode0, i32 0
   %8 = bitcast <2 x i32> %7 to i64
   %9 = inttoptr i64 %8 to [4294967295 x i8] addrspace(4)*
   %10 = call i32 @llvm.amdgcn.reloc.constant(metadata !4)
   %11 = zext i32 %10 to i64
   %12 = getelementptr [4294967295 x i8], [4294967295 x i8] addrspace(4)* %9, i64 0, i64 %11
   %13 = bitcast i8 addrspace(4)* %12 to <4 x i32> addrspace(4)*, !amdgpu.uniform !5
   %14 = load <4 x i32>, <4 x i32> addrspace(4)* %13, align 16, !invariant.load !5
   %15 = call <4 x i32> @llvm.amdgcn.s.buffer.load.v4i32(<4 x i32> %14, i32 0, i32 0)
   call void @llvm.amdgcn.raw.buffer.store.v4i32(<4 x i32> %15, <4 x i32> %14, i32 0, i32 0, i32 0)
   ret void
 }
 
+; Make sure we match constant bases with register offests, in which case
+; the base may be the RHS operand of the load in SDAG.
+; GCN-LABEL: name: test_complex_reg_offset
+; SDAG-DAG: %[[BASE:.*]]:sreg_64 = SI_PC_ADD_REL_OFFSET target-flags(amdgpu-rel32-lo) @0 + 4,
+; SDAG-DAG: %[[OFFSET:.*]]:sreg_32 = S_LSHL_B32
+; SDAG: S_LOAD_DWORD_SGPR killed %[[BASE]], killed %[[OFFSET]],
+; GISEL-DAG: $[[BASE0:.*]] = S_ADD_U32 internal $sgpr0, target-flags(amdgpu-rel32-lo) @0 + 4,
+; GISEL-DAG: $[[BASE1:.*]] = S_ADDC_U32 internal $sgpr1, target-flags(amdgpu-rel32-hi) @0 + 12,
+; GISEL-DAG: $[[OFFSET:.*]] = S_LSHL_B32
+; GISEL-NOT: [[OFFSET]] =
+; GISEL: S_LOAD_DWORD_SGPR killed renamable $[[BASE0]]_[[BASE1]], killed renamable $[[OFFSET]],
+define amdgpu_ps void @test_complex_reg_offset(float addrspace(1)* %out) {
+  %i = load i32, i32 addrspace(4)* @1
+  %i1 = and i32 %i, 3
+  %i2 = zext i32 %i1 to i64
+  %i3 = getelementptr [4 x <2 x float>], [4 x <2 x float>] addrspace(4)* @0, i64 0, i64 %i2, i64 0
+  %i4 = load float, float addrspace(4)* %i3, align 4
+  store float %i4, float addrspace(1)* %out
+  ret void
+}
+
 declare void @llvm.amdgcn.raw.buffer.store.v4i32(<4 x i32>, <4 x i32>, i32, i32, i32 immarg) #1
 
 ; Function Attrs: nounwind readnone speculatable
 declare i32 @llvm.amdgcn.reloc.constant(metadata) #3
 
 ; Function Attrs: nounwind readnone speculatable
 declare i64 @llvm.amdgcn.s.getpc() #3