[PATCH] D61436: [AArch64] NFC: Generalize emitFrameOffset to support more than byte offsets.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2968
   if (DestReg == SrcReg && Offset == 0)
     return;
 
----------------
efriedma wrote:
> Are you intentionally performing the "DestReg == SrcReg && Offset == 0" both here and in emitFrameOffset?
No I wasn't, thanks for pointing out!


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3008
+                   .addImm(Sign * (int)ThisVal);
+    if (ShiftSize)
+      MBI = MBI.addImm(
----------------
efriedma wrote:
> "if (ShiftSize)" is always true?
In this patch it is always true. However, in the subsequent patch {D61437} I add a new opcode for SVE where ShiftSize is 0 (on line 2983).


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3014
+    if (NeedsWinCFI) {
+      int Imm = Sign * (int)(ThisVal << LocalShiftSize);
+      if ((DestReg == AArch64::FP && SrcReg == AArch64::SP) ||
----------------
efriedma wrote:
> The usage of "Sign" here is confusing; we always want a positive immediate for the SEH directives.
I'll remove it, and add an assert to check that Sign is always positive.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3045
+  // First emit non-scalable frame offsets, or a simple 'mov'.
+  if (Bytes || (Offset.isZero() && SrcReg != DestReg)) {
+    assert((DestReg != AArch64::SP || Bytes % 16 == 0) &&
----------------
efriedma wrote:
> What is Offset.isZero() supposed to be checking here?
For the case where the StackOffset can contain both (fixed-size) bytes, and scalable bytes (i.e. MVT::nxv1i8 for SVE where we know there are 'n x bytes', with 'n' being a runtime constant), we want to use ADDXri to emit a frame-offset if:
- Bytes are non-zero (which would mean a meaningful "add" for the fixed-size part of the stack offset)
Or:
- To copy the value of SrcReg to DestReg, but only if StackOffset is empty in its entirety (checking that Bytes are non-zero is not enough).

The functionality is added in {D61437} (lines 3067-3082).


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

https://reviews.llvm.org/D61436