[PATCH] D12337: [Codegen] Ensure stack is properly aligned for call argument initialization

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:204
@@ -203,2 +203,3 @@
   unsigned StackOffset;
+  unsigned MinStackAlign;
   SmallVector<uint32_t, 16> UsedRegs;
----------------
I think this is really more like `MaxStackArgAlign`. It's the maximum alignment of arguments that ended up on the stack, right?

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:273
@@ -271,2 +272,3 @@
   bool isVarArg() const { return IsVarArg; }
 
+  unsigned getNextStackOffset() const {
----------------
We should probably put some Doxygen here about what NextStackOffset really means.

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:274
@@ -272,2 +273,3 @@
 
-  unsigned getNextStackOffset() const { return StackOffset; }
+  unsigned getNextStackOffset() const {
+    return ((StackOffset + MinStackAlign - 1) & ~(MinStackAlign - 1));
----------------
Are you sure this is the right place to change? Isn't this method used to figure out where variadic argument packs start also? Shouldn't we *not* align StackOffset for that use case?

I think it also gets used when we have a reserved stack frame (i.e. no dynamic alloca). As written, your code will overallocate some extra padding that isn't necessary.

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:275
@@ +274,3 @@
+  unsigned getNextStackOffset() const {
+    return ((StackOffset + MinStackAlign - 1) & ~(MinStackAlign - 1));
+  }
----------------
Use RoundUpToAlignment.

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:406
@@ -402,3 +405,3 @@
     assert(Align && ((Align - 1) & Align) == 0); // Align is power of 2.
     StackOffset = ((StackOffset + Align - 1) & ~(Align - 1));
     unsigned Result = StackOffset;
----------------
RoundUpToAlignment here would be a nice cleanup.

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:409
@@ -405,2 +408,3 @@
     StackOffset += Size;
+    MinStackAlign = Align > MinStackAlign ? Align : MinStackAlign;
     MF.getFrameInfo()->ensureMaxAlignment(Align);
----------------
This looks like `std::max(Align, MinStackAlign)` to me.


http://reviews.llvm.org/D12337