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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2968
   if (DestReg == SrcReg && Offset == 0)
     return;
 
----------------
Are you intentionally performing the "DestReg == SrcReg && Offset == 0" both here and in emitFrameOffset?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3008
+                   .addImm(Sign * (int)ThisVal);
+    if (ShiftSize)
+      MBI = MBI.addImm(
----------------
"if (ShiftSize)" is always true?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3014
+    if (NeedsWinCFI) {
+      int Imm = Sign * (int)(ThisVal << LocalShiftSize);
+      if ((DestReg == AArch64::FP && SrcReg == AArch64::SP) ||
----------------
The usage of "Sign" here is confusing; we always want a positive immediate for the SEH directives.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3022
+              .addImm(Imm)
+              .setMIFlag(Flag);
+      } else if (DestReg == AArch64::SP) {
----------------
We should assert here that the remaining offset is zero, to make the expected control flow clear.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:3026
+            .addImm(Imm)
+            .setMIFlag(Flag);
+      }
----------------
We should assert here that SrcReg is SP.


================
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) &&
----------------
What is Offset.isZero() supposed to be checking here?


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

https://reviews.llvm.org/D61436