[PATCH] D111133: [AARCH64] Teach AArch64FrameLowering::getFrameIndexReferencePreferSP really prefer SP.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Spent some time educating myself on AArch64 frame lowering.  I'm mostly relying on the diagram at the top of AArch64FrameLowering.cpp.  Review guidance is about specializing the case where the variable sized regions are empty.



================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:3536
   }
+  if (!MFI.hasVarSizedObjects()) {
+    FrameReg = AArch64::SP;
----------------
skatkov wrote:
> Probably I need to check hasStackRealignment as well...
You do.  

I'd also strongly prefer you specialize this for the case where there is no SVE stack.  (e.g !getSVEStackSize()) Support for SVE can be added at a later time if needed, and would need it's own test coverage.  I have no clue how to even suggest writing such a test at this time.

Once you specialize, the code here gets a lot simpler.  


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:3550
 
   return getFrameIndexReference(MF, FI, FrameReg);
 }
----------------
You should probably use an early return with the inverted condition above.  This would reduce nesting in the new code.  Once you simplify based on no SVE, it may be more of a wash though...


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

https://reviews.llvm.org/D111133