[AArch64] Correct isLegalAddressingMode for ldp/stp

[bcl5980]

Summary

AArch64 doesn't support ldp/stp with scale register. So if the load/store instruction may be selected to ldp/stp, return illegal addressing mode when the address pattern has scale register.

Fix https://github.com/llvm/llvm-project/issues/53877

[bcl5980]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

13217

I'm not sure if we have better way to check here. Can someone help to find some other clean solution?

[bcl5980]

llvm/test/Transforms/LoopStrengthReduce/AArch64/issue53877.ll

1

I will update the baseline if reviewers look good for the test.

[dmgreen]

This is an area of the compiler that is very performance sensitive. We have known there is room for improvement here for a while, but what you are adding is a heuristic built on top of other heuristics, and it can be difficult to make improvements without causing other issues. This patch looks sensible to me, but do you have any performance results on a larger set of results?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

13217

From what I can tell, you mean that the Ty is a vector, loaded into a FPR? Not really a 128bit scalar type. There is an LDR q [r,r] addressing mode, so we are adding a heuristic saying it is better to assume they will later become an ldp, which don't have a rr addressing mode.

It would probably be best to check it is a vector directly. Or maybe have the isLegalAddressingMode return more precise results (as it does at the moment) and teach the next layer up (LSR) about paired addressing modes.

llvm/test/Transforms/LoopStrengthReduce/AArch64/issue53877.ll

1

Can you give it a better name? And do some cleanup of "; Function Attrs: mustprogress..", etc.

[bcl5980]

In D124014#3461164, @dmgreen wrote:

This is an area of the compiler that is very performance sensitive. We have known there is room for improvement here for a while, but what you are adding is a heuristic built on top of other heuristics, and it can be difficult to make improvements without causing other issues. This patch looks sensible to me, but do you have any performance results on a larger set of results?

Thanks for the detail explaination. I agree addressing mode legal check is perf sensitive.
I'm a beginner so I don't know we already have some plan to improve here. I don't have any real perf data. So I will abandon the patch if someone is working on it.
And can you tell me if we do some perf sensitive change. what should we test ? I know some CPU perf bench like specCPU, cinebench, geekbench, but all of them are not open-source.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

13217

The purpose of this code is find the type will be select to ldp/stp.
What I thought is i128 also can be compile to stp/ldp with 2 int registers so the type is scalar of vector is not the key point.
And SVE load store may not come to here as line 13183

if (isa<ScalableVectorType>(Ty)) {

So we need to fix the FixedVectorType with 128bits FP register here also. Am I right?

[dmgreen]

Hmm. So I think that the current version of isLegalAddressingMode is technically correct - in that AArch64 does have r+r addressing modes for both scalars and vectors. (It might not for strange types, but that's not what we are targeting here). Adding a heuristic to make it wrong, so that the next level up produces better code is an option - but it may be better in the long run to fix the places using this (LSR for example) so that they understand about paired loads and can produce better code in all cases.

And can you tell me if we do some perf sensitive change. what should we test ? I know some CPU perf bench like specCPU, cinebench, geekbench, but all of them are not open-source.

There are the benchmarks in the llvm-test-suite. They can be a bit noisy though, unfortunately, so need to be ran with care.

[bcl5980]

In D124014#3464520, @dmgreen wrote:

Hmm. So I think that the current version of isLegalAddressingMode is technically correct - in that AArch64 does have r+r addressing modes for both scalars and vectors. (It might not for strange types, but that's not what we are targeting here). Adding a heuristic to make it wrong, so that the next level up produces better code is an option - but it may be better in the long run to fix the places using this (LSR for example) so that they understand about paired loads and can produce better code in all cases.

And can you tell me if we do some perf sensitive change. what should we test ? I know some CPU perf bench like specCPU, cinebench, geekbench, but all of them are not open-source.

There are the benchmarks in the llvm-test-suite. They can be a bit noisy though, unfortunately, so need to be ran with care.

Thanks，I will abandon this change.

[bcl5980]

@dmgreen , do I need to add a todo and check in the test after clean?

[dmgreen]

In D124014#3470289, @bcl5980 wrote:

@dmgreen , do I need to add a todo and check in the test after clean?

I'm not sure about a TODO. Where would it be added? I find they tend to bitrot and muddle the code, more then they help sometimes.

Having a test sounds useful, to show inefficiencies that can be fixed.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

if (!AM.Scale) {
     unsigned shift = Log2_64(NumBytes);
     if (NumBytes && Offset > 0 && (Offset / NumBytes) <= (1LL << 12) - 1 &&
         // Must be a multiple of NumBytes (NumBytes is a power of 2)
         (Offset >> shift) << shift == Offset)
       return true;
     return false;
   }
 
+  // ldp/stp don't support scale
+  if (Ty->isSized() && DL.getTypeSizeInBits(Ty) > 64)
+    return false;
+
   // Check reg1 + SIZE_IN_BYTES * reg2 and reg1 + reg2
 
   return AM.Scale == 1 || (AM.Scale > 0 && (uint64_t)AM.Scale == NumBytes);
 }
 
 bool AArch64TargetLowering::shouldConsiderGEPOffsetSplit() const {
   // Consider splitting large offset of struct or array.
   return true;

llvm/test/Transforms/LoopStrengthReduce/AArch64/issue53877.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s
+
+; Function Attrs: mustprogress nofree nosync nounwind uwtable
+define dso_local void @convolution(ptr %src0, ptr %src1, i64 %stride_xm, i64 %stride_xp, ptr %dst, i32 %w) {
+; CHECK-LABEL: convolution:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:  .LBB0_1: // %do.body
+; CHECK-NEXT:    // =>This Inner Loop Header: Depth=1
+; CHECK-NEXT:    ldr q0, [x0, x2]
+; CHECK-NEXT:    subs w5, w5, #1
+; CHECK-NEXT:    ldr q1, [x0, x3]
+; CHECK-NEXT:    ldp q2, q3, [x0], #32
+; CHECK-NEXT:    fadd v0.4s, v0.4s, v1.4s
+; CHECK-NEXT:    ldr q4, [x1, x2]
+; CHECK-NEXT:    ldr q5, [x1, x3]
+; CHECK-NEXT:    ldp q6, q1, [x1], #32
+; CHECK-NEXT:    fadd v2.4s, v2.4s, v3.4s
+; CHECK-NEXT:    fadd v3.4s, v4.4s, v5.4s
+; CHECK-NEXT:    fadd v1.4s, v6.4s, v1.4s
+; CHECK-NEXT:    fadd v0.4s, v0.4s, v2.4s
+; CHECK-NEXT:    fadd v1.4s, v3.4s, v1.4s
+; CHECK-NEXT:    fadd v0.4s, v0.4s, v1.4s
+; CHECK-NEXT:    str q0, [x4], #16
+; CHECK-NEXT:    b.ne .LBB0_1
+; CHECK-NEXT:  // %bb.2: // %do.end
+; CHECK-NEXT:    ret
+entry:
+  br label %do.body
+
+do.body:                                          ; preds = %do.body, %entry
+  %dst.addr.0 = phi ptr [ %dst, %entry ], [ %incdec.ptr, %do.body ]
+  %src1.addr.0 = phi ptr [ %src1, %entry ], [ %incdec.ptr2.i7, %do.body ]
+  %src0.addr.0 = phi ptr [ %src0, %entry ], [ %incdec.ptr2.i, %do.body ]
+  %w.addr.0 = phi i32 [ %w, %entry ], [ %dec, %do.body ]
+  %add.ptr.i = getelementptr inbounds i8, ptr %src0.addr.0, i64 %stride_xm
+  %0 = load <4 x float>, ptr %add.ptr.i, align 16
+  %add.ptr1.i = getelementptr inbounds i8, ptr %src0.addr.0, i64 %stride_xp
+  %1 = load <4 x float>, ptr %add.ptr1.i, align 16
+  %incdec.ptr.i = getelementptr inbounds <4 x float>, ptr %src0.addr.0, i64 1
+  %2 = load <4 x float>, ptr %src0.addr.0, align 16
+  %incdec.ptr2.i = getelementptr inbounds <4 x float>, ptr %src0.addr.0, i64 2
+  %3 = load <4 x float>, ptr %incdec.ptr.i, align 16
+  %add.i = fadd <4 x float> %0, %1
+  %add3.i = fadd <4 x float> %2, %3
+  %add4.i = fadd <4 x float> %add.i, %add3.i
+  %add.ptr.i4 = getelementptr inbounds i8, ptr %src1.addr.0, i64 %stride_xm
+  %4 = load <4 x float>, ptr %add.ptr.i4, align 16
+  %add.ptr1.i5 = getelementptr inbounds i8, ptr %src1.addr.0, i64 %stride_xp
+  %5 = load <4 x float>, ptr %add.ptr1.i5, align 16
+  %incdec.ptr.i6 = getelementptr inbounds <4 x float>, ptr %src1.addr.0, i64 1
+  %6 = load <4 x float>, ptr %src1.addr.0, align 16
+  %incdec.ptr2.i7 = getelementptr inbounds <4 x float>, ptr %src1.addr.0, i64 2
+  %7 = load <4 x float>, ptr %incdec.ptr.i6, align 16
+  %add.i8 = fadd <4 x float> %4, %5
+  %add3.i9 = fadd <4 x float> %6, %7
+  %add4.i10 = fadd <4 x float> %add.i8, %add3.i9
+  %add = fadd <4 x float> %add4.i, %add4.i10
+  %incdec.ptr = getelementptr inbounds <4 x float>, ptr %dst.addr.0, i64 1
+  store <4 x float> %add, ptr %dst.addr.0, align 16
+  %dec = add nsw i32 %w.addr.0, -1
+  %tobool.not = icmp eq i32 %dec, 0
+  br i1 %tobool.not, label %do.end, label %do.body
+
+do.end:                                           ; preds = %do.body
+  ret void
+}