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

[Daniel Sanders]

LGTM with the nits fixed and a warning for the 'needs realignment but can't realign' case.

We'll need to follow up on the overallocation issue but suboptimal is better than non-functional.


================
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 {
----------------
dsanders wrote:
> Nit? MFI->isFrameAddressTaken() doesn't seem to come under any of these conditions.
Done

================
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);
----------------
vkalintiris wrote:
> dsanders wrote:
> > Nit: This is the same as hasBP() we should use that if we can to avoid code duplication.
> We need access to a MipsFrameLowering object in order to use hasBP(). I'm not sure but I don't think we can get a reference to this object from inside MipsRegisterInfo.
In that case, could you add a comment indicating it should be the same as hasBP()

================
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);
 }
----------------
dsanders wrote:
> Nit: We should explain these conditions.
Done

================
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;
+}
----------------
vkalintiris wrote:
> dsanders wrote:
> > 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.
> I agree with you on that. IMHO, a better name would be shouldRealignStack() or something similar. I kept this name because I wanted to be consistent with the other targets.
Both spellings have the same issue. If the answer to 'Are we able to realign the stack?' is 'no' then needsStackRealignment()/shouldRealignStack() can return false even though the answer to 'Do we need stack realignment?' is yes.

We should emit a warning when this happens since the users code is very likely to break in this situation.

================
Comment at: lib/Target/Mips/MipsRegisterInfo.cpp:315-321
@@ +314,9 @@
+
+  // Avoid realigning functions that explicitly do not want to be realigned.
+  // Normally, we should report an error when a function should be dynamically
+  // realigned but also has the attribute no-realign-stack. Unfotunately,
+  // with this attirbute, MachineFrameInfo clamps each new object's alignment
+  // to that of the stack's alignment as specified by the ABI. As a result,
+  // the information of whether we have objects with larger alignment
+  // requirement than the stack's alignment is already lost at this point.
+  if (MF.getFunction()->hasFnAttribute("no-realign-stack"))
----------------
A couple typos:
Unfotunately -> Unfortunately
attirbute -> attribute

================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:143
@@ +142,3 @@
+    if (MFI->hasVarSizedObjects() && !MFI->isFixedObjectIndex(FrameIndex))
+      FrameReg = ABI.IsN64() ? Mips::S7_64 : Mips::S7;
+    else if (MFI->isFixedObjectIndex(FrameIndex))
----------------
Nit: Instead of:
  ABI.IsN64() ? Mips::S7_64 : Mips::S7;
add a MipsABIInfo::GetBasePtr() and use that.

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

================
Comment at: test/CodeGen/Mips/dynamic-stack-realignment.ll:30
@@ +29,3 @@
+  ; prologue
+  ; GP32:       addiu   $sp, $sp, -1024
+  ; GP32:       sw      $ra, 1020($sp)
----------------
Nit: Please document the overallocation with a FIXME. This particular case needs a frame of up to between 16 and 512-bytes but currently allocates between 1024 and 1536 bytes.

Likewise for the other examples below.

================
Comment at: test/CodeGen/Mips/dynamic-stack-realignment.ll:17
@@ +16,3 @@
+
+define void @func_01() {
+entry:
----------------
dsanders wrote:
> 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.
Done

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

================
Comment at: test/CodeGen/Mips/dynamic-stack-realignment.ll:233-259
@@ +232,28 @@
+
+; Check that we do not perform dynamic stack realignment in the presence of
+; the "no-realign-stack" function attribute.
+define void @func_09() "no-realign-stack" {
+entry:
+; ALL-LABEL: func_09:
+
+  ; ALL-NOT:  and     $sp, $sp, $[[T0:[0-9]+|ra|gp]]
+
+  %a = alloca i32, align 512
+  call void @helper_01(i32 0, i32 0, i32 0, i32 0, i32* %a)
+  ret void
+}
+
+define void @func_10(i32 %sz) "no-realign-stack" {
+entry:
+; ALL-LABEL: func_10:
+
+  ; ALL-NOT:  and     $sp, $sp, $[[T0:[0-9]+|ra|gp]]
+
+  %a0 = alloca i32, i32 %sz, align 512
+  %a1 = alloca i32, align 4
+
+  store volatile i32 111, i32* %a0, align 512
+  store volatile i32 222, i32* %a1, align 4
+
+  ret void
+}
----------------
dsanders wrote:
> Shouldn't these two cases be errors? We need to align to a large alignment but we can't because we've been told not to realign the stack.
Done (MipsRegisterInfo::needsStackRealignment() has a comment explaining why they aren't errors).

http://reviews.llvm.org/D8633

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