[PATCH] D124014: [AArch64] Correct isLegalAddressingMode for ldp/stp

Wed Apr 20 01:06:58 PDT 2022

bcl5980 added a comment.

In D124014#3461164 <https://reviews.llvm.org/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.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13217
+  // ldp/stp don't support scale
+  if (Ty->isSized() && DL.getTypeSizeInBits(Ty) > 64)
+    return false;
----------------
dmgreen wrote:
> bcl5980 wrote:
> > I'm not sure if we have better way to check here. Can someone help to find some other clean solution?
> 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.
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?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124014/new/

https://reviews.llvm.org/D124014