[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:274-279
@@ -272,4 +273,8 @@
 
-  unsigned getNextStackOffset() const { return StackOffset; }
+  /// getNextStackOffset - Return the stack offset needed to be able to store
+  /// all stack slots according to their alignment requirements.
+  unsigned getNextStackOffset() const {
+    return RoundUpToAlignment(StackOffset, MaxStackArgAlign);
+  }
 
   /// isAllocated - Return true if the specified register (or an alias) is
----------------
jketema wrote:
> jketema wrote:
> > rnk wrote:
> > > Yes, but here's the 32-bit test case I'm imagining:
> > >   void f(__m128 v, const char *f, ...) {
> > >     va_list ap;
> > >     va_start(ap, f);
> > >     int d = va_arg(ap, int);
> > >   }
> > > Do we have padding between 'f' and the variadic int parameter? I think the answer should be know, because if they were not variadic, they would not have padding between them.
> > > 
> > > 
> > Sorry, I missed this before I bumped. Somehow the system didn't sent me a notification email.
> > 
> > I don't see why this is a problem, my patch only changes the alignment of the whole frame, not the padding between the individual members; the computation for that stays unchanged. Hence, there will only be additional padding after the very last of the variadic parameters supplied.
> Reading back all the comments, I think I see what you're getting at: What you're saying is that `getNextStackOffset` is also used to compute the offset of the variadic arguments with respect to the beginning of the stack frame?
> 
> If that's the case, then the patch would indeed be wrong, and I think we would then need both the old variant and the new one I'm proposing and adapt every use accordingly. Or do you see a better solution?
Exactly, `getNextStackOffset` currently has more than one use. You should take the example C code above and turn it into a test case for lit to show that at least this use isn't changing.

Given that there is more than one use, the I think we should introduce a new method called `getAlignedCallFrameSize`, update the appropriate call sites that use this in PEI, and declare victory. :)


http://reviews.llvm.org/D12337