[PATCH] [mips] Add support for dynamic stack realignment.

Daniel Sanders daniel.sanders at imgtec.com
Wed Apr 1 07:50:43 PDT 2015


I haven't finished looking at func_03 - func_10 in the test case yet but here's my comments so far.

Could you add some tests that check the debug info for variables affected by this? I'd like to be sure that the location information is correct.


================
Comment at: lib/Target/Mips/MipsFrameLowering.cpp:92-95
@@ -91,5 +91,6 @@
 
 // hasFP - Return true if the specified function should have a dedicated frame
-// pointer register.  This is true if the function has variable sized allocas or
-// if frame pointer elimination is disabled.
+// pointer register.  This is true if the function has variable sized allocas,
+// if it needs dynamic stack realignment, or if frame pointer elimination is
+// disabled.
 bool MipsFrameLowering::hasFP(const MachineFunction &MF) const {
----------------
Nit? MFI->isFrameAddressTaken() doesn't seem to come under any of these conditions.

================
Comment at: lib/Target/Mips/MipsRegisterInfo.cpp:185-186
@@ +184,4 @@
+      // allocate variable-sized objects at runtime.
+      if (needsStackRealignment(MF) &&
+          MF.getFrameInfo()->hasVarSizedObjects()) {
+        Reserved.set(Mips::S7);
----------------
Nit: This is the same as hasBP() we should use that if we can to avoid code duplication.

================
Comment at: lib/Target/Mips/MipsRegisterInfo.cpp:290-302
@@ -274,2 +289,15 @@
+
+  if (!Subtarget.hasStandardEncoding())
+    return false;
+
+  if (MF.getFunction()->hasFnAttribute("no-realign-stack"))
+    return false;
+
+  if (!MF.getRegInfo().canReserveReg(FP))
+    return false;
 
+  if (Subtarget.getFrameLowering()->hasReservedCallFrame(MF))
+    return true;
+
+  return MF.getRegInfo().canReserveReg(BP);
 }
----------------
Nit: We should explain these conditions.

================
Comment at: lib/Target/Mips/MipsRegisterInfo.cpp:305-320
@@ -276,1 +304,17 @@
 
+bool MipsRegisterInfo::needsStackRealignment(const MachineFunction &MF) const {
+  const MipsSubtarget &Subtarget = MF.getSubtarget<MipsSubtarget>();
+  const MachineFrameInfo *MFI = MF.getFrameInfo();
+
+  bool CanRealign = canRealignStack(MF);
+
+  const Function *F = MF.getFunction();
+  if (F->hasFnAttribute(Attribute::StackAlignment))
+    return CanRealign;
+
+  unsigned StackAlignment = Subtarget.getFrameLowering()->getStackAlignment();
+  if (MFI->getMaxAlignment() > StackAlignment)
+    return CanRealign;
+
+  return false;
+}
----------------
I think I may be missing something here. The code seems to be saying that if we need realignment but can't do achieve it, then we don't really need it. Shouldn't that at least emit a warning?

I see other targets are silently doing the same thing, so this may be intended but it seems odd.

================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:142
@@ +141,3 @@
+    if (MFI->hasVarSizedObjects() && !MFI->isFixedObjectIndex(FrameIndex))
+      FrameReg = isN64 ? Mips::S7_64 : Mips::S7;
+    else if (FrameIndex < 0)
----------------
It's not necessary in this patch but we should consider adding inlined members to MipsRegisterInfo rather than duplicating the selection between REG and REG_64 all the time (e.g. getZeroReg(), getFPReg(), getSPReg(), etc.).

================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:143
@@ +142,3 @@
+      FrameReg = isN64 ? Mips::S7_64 : Mips::S7;
+    else if (FrameIndex < 0)
+      FrameReg = getFrameRegister(MF);
----------------
Why 'FrameIndex < 0' and not 'MFI->isFixedObjectIndex(FrameIndex)'?

================
Comment at: test/CodeGen/Mips/dynamic-stack-realignment.ll:17
@@ +16,3 @@
+
+define void @func_01() {
+entry:
----------------
Nit: Could you make it a bit clearer what cases each test covers? I was going to comment that we should check N32/N64's output too but I see that func_02 is covering the N64 case.

================
Comment at: test/CodeGen/Mips/dynamic-stack-realignment.ll:22
@@ +21,3 @@
+  ; prologue
+  ; GP32:       addiu   $sp, $sp, -1024
+  ; GP32:       sw      $ra, 1020($sp)
----------------
I don't think -1024 is correct (but the program will work as far as I can tell). The frame will contain the 8-bytes for $ra/$fp, up to 504-bytes padding, 4-bytes for the alloca'd i32, 4-bytes for the 5th argument to helper_01, and 16-bytes for the reserved space, giving a total of between 32 and 536 bytes for the frame.

At the moment, we seem to be allocating 1024 bytes as well as up to 504 bytes padding. It looks like we're rounding the size of each region of the frame up to the maximum alignment.

We're discussing off-list whether the base pointer and locals with large alignments should be where they currently are. Some of the above issues may be related to the current position of this pointer and region.

================
Comment at: test/CodeGen/Mips/dynamic-stack-realignment.ll:48
@@ +47,3 @@
+
+define void @func_02() {
+entry:
----------------
We should check the N32 case too.

http://reviews.llvm.org/D8633

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list