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

Wed Apr 20 00:23:49 PDT 2022

dmgreen added a comment.

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?

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13217
+  // ldp/stp don't support scale
+  if (Ty->isSized() && DL.getTypeSizeInBits(Ty) > 64)
+    return false;
----------------
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.

================
Comment at: llvm/test/Transforms/LoopStrengthReduce/AArch64/issue53877.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s
----------------
bcl5980 wrote:
> I will update the baseline if reviewers look good for the test.
Can you give it a better name? And do some cleanup of "; Function Attrs: mustprogress..", etc.

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

https://reviews.llvm.org/D124014