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

Vasileios Kalintiris Vasileios.Kalintiris at imgtec.com
Tue Apr 21 07:26:41 PDT 2015


================
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);
----------------
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.

================
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;
+}
----------------
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.

http://reviews.llvm.org/D8633

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






More information about the llvm-commits mailing list