[PATCH] D14871: [Power PC] fix calculating address of arguments on stack for variadic functions

[John McCall via cfe-commits]

rjmccall added inline comments.

================
Comment at: lib/CodeGen/TargetInfo.cpp:3548
@@ +3547,3 @@
+    llvm::Value *OverflowArgArea = OverflowArea.getPointer();
+    uint32_t Align = CGF.getContext().getTypeAlignInChars(Ty).getQuantity();
+    if (Align > 4) {
----------------
This patch applies to more types than just double.  You should probably add tests for some of the other cases, but if not, please at least make your commit message convey the intended generality.

================
Comment at: lib/CodeGen/TargetInfo.cpp:3549
@@ +3548,3 @@
+    uint32_t Align = CGF.getContext().getTypeAlignInChars(Ty).getQuantity();
+    if (Align > 4) {
+      // OverflowArgArea = (OverflowArgArea + Align - 1) & -Align;
----------------
if (Align > OverflowAreaAlign)

================
Comment at: lib/CodeGen/TargetInfo.cpp:3559
@@ +3558,3 @@
+                                   OverflowArgArea->getType(),
+                                   "overflow_arg_area.align");
+    }
----------------
Would you mind extracting a function to do this realignment?  There are 3-4 other places in this file that do exactly this.  Using the existing code pattern from emitVoidPtrDirectVAArg — i.e. using an add instead of a non-inbounds GEP — seems reasonable.

I think this is a sensible prototype:
  static Address emitRoundPointerUpToAlignment(CodeGenFunction &CGF, llvm::Value *Ptr, CharUnits Align, const llvm::Twine &Name);

================
Comment at: lib/CodeGen/TargetInfo.cpp:3562
@@ -3549,1 +3561,3 @@
+ 
+    OverflowArea = Address(OverflowArgArea, CharUnits::fromQuantity(Align));
     MemAddr = Builder.CreateElementBitCast(OverflowArea, DirectTy);
----------------
This code will be more readable if you keep Align as a CharUnits.


http://reviews.llvm.org/D14871