[PATCH] D18619: [AArch64] Combine callee-save and local stack SP adjustment instructions.

[Geoff Berry via llvm-commits]

gberry added a comment.

Tim,

Thanks, we're almost there :)


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:324
@@ +323,3 @@
+  unsigned NewOpc;
+  bool NewIsDec = false;
+  bool NewIsUnscaled = false;
----------------
t.p.northover wrote:
> Perhaps use MBBI->isStore()? No obvious way to infer NewIsUnscaled, unfortunately.
Yeah, that seems better.

================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:395-397
@@ +394,5 @@
+// combined SP bump by adding the local stack size to the stack offsets.
+static MachineBasicBlock::iterator fixupCalleeSaveRestoreStackOffsetAndIncDec(
+    MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI, DebugLoc DL,
+    const TargetInstrInfo *TII, unsigned LocalStackSize) {
+
----------------
t.p.northover wrote:
> I think this function could just be
> 
>     static void fixupCalleeSaveRestoreStackOffsetIncAndDec(MachineInstr *MI, unsigned LocalStackSize) {
>       MachineOperand &MO = MI->getOperand(MI->getNumExplicitOperands() - 1);
>       assert(LocalStackSize % 8 == 0);
>       MO.setImm(MO.getImm() + LocalStackSize/8);
>     }
> 
> But you probably made the right call in `convertCalleeSaveRestoreToSPPrePostIncDec`: the pre/post versions have extra operands that would get messy.
Yep, this was just my lack of grep skills/knowledge of the MI API.  I looked for a setOperand to do just what you are suggesting, without realizing I could just change the immediate value.

================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.cpp:391-394
@@ -390,3 +390,6 @@
   // Modify MI as necessary to handle as much of 'Offset' as possible
-  Offset = TFI->resolveFrameIndexReference(MF, FrameIndex, FrameReg);
+  bool ForceSP = MI.getFlag(MachineInstr::FrameSetup) ||
+                 MI.getFlag(MachineInstr::FrameDestroy);
+  Offset = TFI->resolveFrameIndexReference(MF, FrameIndex, FrameReg,
+                                           /*PreferFP=*/false, ForceSP);
   if (rewriteAArch64FrameIndex(MI, FIOperandNum, FrameReg, Offset, TII))
----------------
t.p.northover wrote:
> Is this left-over from when you tried to convert spill/restoreCalleeSavedRegisters to use "addFrameIndex"? I can't see the need for it in the present scheme (no FrameSetup/FrameDestroy instructions seem to be created with a FrameIndex operand).
Yep, I was right to get rid of it the first time :)


http://reviews.llvm.org/D18619