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

[spetrovic]

Summary

This patch fixes calculating address of arguments larger than 32 bit on stack for variadic functions (rounding up address to alignment) on ppc32 architecture.

[hfinkel]

lib/CodeGen/TargetInfo.cpp

3548

You need to use variable names consistent with our coding convention (OverflowArgArea, etc.).

[rjmccall]

lib/CodeGen/TargetInfo.cpp

3549

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.

3550

if (Align > OverflowAreaAlign)

3560

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

3563

This code will be more readable if you keep Align as a CharUnits.

[rjmccall]

lib/CodeGen/TargetInfo.cpp

251

Thank you for extracting this.

First, this function deserves a doc comment now; I would suggest:

/// Dynamically round a pointer up to a multiple of the given alignment.

Also, this is now generically useful, so (1) please rename OverflowArgArea to something more generic, like Ptr, and (2) please use LLVM value names based on the name already in Ptr. That is, instead of "overflow_arg_area.align", please use

Ptr->getName () + ".aligned"

Finally, please look for the other places in this file that you could change to use this new function. There's one in emitVoidPtrDirectVAArg, and there are several other in other targets.

3552

Please sink OverflowArgArea into the if block, since you don't need it outside.

[spetrovic]

Comments addressed.

[rjmccall]

Thank you. A few more style complaints; with those fixed, LGTM.

lib/CodeGen/TargetInfo.cpp

166

Minor code-style nitpick: please put the * next to the function name, not the type.

221

LLVM code style is to always leave the braces there.

3092

Braces.

[spetrovic]

Comments addressed.

[rjmccall]

Please feel free to commit after you make this last change.

lib/CodeGen/TargetInfo.cpp

219

LLVM code style is to put the else on the same line as the closing brace.

lib/CodeGen/TargetInfo.cpp

case Indirect:
     break;
   case Expand:
     OS << "Expand";
     break;
   }
   OS << ")\n";
 }
 
+// Dynamically round a pointer up to a multiple of the given alignment.
+static llvm::Value *emitRoundPointerUpToAlignment(CodeGenFunction &CGF,
+                                                  llvm::Value *Ptr,
+                                                  CharUnits Align) {
+  llvm::Value *PtrAsInt = Ptr;
+  // OverflowArgArea = (OverflowArgArea + Align - 1) & -Align;
+  PtrAsInt = CGF.Builder.CreatePtrToInt(PtrAsInt, CGF.IntPtrTy);
+  PtrAsInt = CGF.Builder.CreateAdd(PtrAsInt,
+        llvm::ConstantInt::get(CGF.IntPtrTy, Align.getQuantity() - 1));
+  PtrAsInt = CGF.Builder.CreateAnd(PtrAsInt,
+           llvm::ConstantInt::get(CGF.IntPtrTy, -Align.getQuantity()));
+  PtrAsInt = CGF.Builder.CreateIntToPtr(PtrAsInt,
+                                        Ptr->getType(),
+                                        Ptr->getName() + ".aligned");
+  return PtrAsInt;
+}
+
 /// Emit va_arg for a platform using the common void* representation,
 /// where arguments are simply emitted in an array of slots on the stack.
 ///
 /// This version implements the core direct-value passing rules.
 ///
 /// \param SlotSize - The size and alignment of a stack slot.
 ///   Each argument will be allocated to a multiple of this number of
 ///   slots, and all the slots will be aligned to this value.
static Address emitVoidPtrDirectVAArg(CodeGenFunction &CGF,
   if (VAListAddr.getElementType() != CGF.Int8PtrTy)
     VAListAddr = CGF.Builder.CreateElementBitCast(VAListAddr, CGF.Int8PtrTy);
 
   llvm::Value *Ptr = CGF.Builder.CreateLoad(VAListAddr, "argp.cur");
 
   // If the CC aligns values higher than the slot size, do so if needed.
   Address Addr = Address::invalid();
   if (AllowHigherAlign && DirectAlign > SlotSize) {
-    llvm::Value *PtrAsInt = Ptr;
-    PtrAsInt = CGF.Builder.CreatePtrToInt(PtrAsInt, CGF.IntPtrTy);
-    PtrAsInt = CGF.Builder.CreateAdd(PtrAsInt,
-          llvm::ConstantInt::get(CGF.IntPtrTy, DirectAlign.getQuantity() - 1));
-    PtrAsInt = CGF.Builder.CreateAnd(PtrAsInt,
-             llvm::ConstantInt::get(CGF.IntPtrTy, -DirectAlign.getQuantity()));
-    Addr = Address(CGF.Builder.CreateIntToPtr(PtrAsInt, Ptr->getType(),
-                                              "argp.cur.aligned"),
+    Addr = Address(emitRoundPointerUpToAlignment(CGF, Ptr, DirectAlign),
                                                  DirectAlign);
   } else {
-    Addr = Address(Ptr, SlotSize);
+    Addr = Address(Ptr, SlotSize); 
   }
 
   // Advance the pointer past the argument, then store that back.
   CharUnits FullDirectSize = DirectSize.RoundUpToAlignment(SlotSize);
   llvm::Value *NextPtr =
     CGF.Builder.CreateConstInBoundsByteGEP(Addr.getPointer(), FullDirectSize,
                                            "argp.next");
   CGF.Builder.CreateStore(NextPtr, VAListAddr);
 
   // If the argument is smaller than a slot, and this is a big-endian
   // target, the argument will be right-adjusted in its slot.
   if (DirectSize < SlotSize && CGF.CGM.getDataLayout().isBigEndian()) {
     Addr = CGF.Builder.CreateConstInBoundsByteGEP(Addr, SlotSize - DirectSize);

 ///   Each argument will be allocated to a multiple of this number of
 ///   slots, and all the slots will be aligned to this value.
 /// \param AllowHigherAlign - The slot alignment is not a cap;
 ///   an argument type with an alignment greater than the slot size
 ///   will be emitted on a higher-alignment address, potentially
 ///   leaving one or more empty slots behind as padding.
 static Address emitVoidPtrVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                 QualType ValueTy, bool IsIndirect,
                                 std::pair<CharUnits, CharUnits> ValueInfo,
                                 CharUnits SlotSizeAndAlign,
                                 bool AllowHigherAlign) {
   // The size and alignment of the value that was passed directly.
   CharUnits DirectSize, DirectAlign;
   if (IsIndirect) {
     DirectSize = CGF.getPointerSize();
     DirectAlign = CGF.getPointerAlign();
   } else {
Address overflow_arg_area_p = CGF.Builder.CreateStructGEP(
       VAListAddr, 2, CharUnits::fromQuantity(8), "overflow_arg_area_p");
   llvm::Value *overflow_arg_area =
     CGF.Builder.CreateLoad(overflow_arg_area_p, "overflow_arg_area");
 
   // AMD64-ABI 3.5.7p5: Step 7. Align l->overflow_arg_area upwards to a 16
   // byte boundary if alignment needed by type exceeds 8 byte boundary.
   // It isn't stated explicitly in the standard, but in practice we use
   // alignment greater than 16 where necessary.
-  uint64_t Align = CGF.getContext().getTypeAlignInChars(Ty).getQuantity();
-  if (Align > 8) {
-    // overflow_arg_area = (overflow_arg_area + align - 1) & -align;
-    llvm::Value *Offset =
-      llvm::ConstantInt::get(CGF.Int64Ty, Align - 1);
-    overflow_arg_area = CGF.Builder.CreateGEP(overflow_arg_area, Offset);
-    llvm::Value *AsInt = CGF.Builder.CreatePtrToInt(overflow_arg_area,
-                                                    CGF.Int64Ty);
-    llvm::Value *Mask = llvm::ConstantInt::get(CGF.Int64Ty, -(uint64_t)Align);
-    overflow_arg_area =
-      CGF.Builder.CreateIntToPtr(CGF.Builder.CreateAnd(AsInt, Mask),
-                                 overflow_arg_area->getType(),
-                                 "overflow_arg_area.align");
+  CharUnits Align = CGF.getContext().getTypeAlignInChars(Ty);
+  if (Align > CharUnits::fromQuantity(8)) {
+    overflow_arg_area = emitRoundPointerUpToAlignment(CGF, overflow_arg_area,
+                                                      Align);
   }
 
   // AMD64-ABI 3.5.7p5: Step 8. Fetch type from l->overflow_arg_area.
   llvm::Type *LTy = CGF.ConvertTypeForMem(Ty);
   llvm::Value *Res =
     CGF.Builder.CreateBitCast(overflow_arg_area,
                               llvm::PointerType::getUnqual(LTy));
 
   // AMD64-ABI 3.5.7p5: Step 9. Set l->overflow_arg_area to:
   // l->overflow_arg_area + sizeof(type).
   // AMD64-ABI 3.5.7p5: Step 10. Align l->overflow_arg_area upwards to
   // an 8 byte boundary.
 
   uint64_t SizeInBytes = (CGF.getContext().getTypeSize(Ty) + 7) / 8;
   llvm::Value *Offset =
       llvm::ConstantInt::get(CGF.Int32Ty, (SizeInBytes + 7)  & ~7);
   overflow_arg_area = CGF.Builder.CreateGEP(overflow_arg_area, Offset,
                                             "overflow_arg_area.next");
   CGF.Builder.CreateStore(overflow_arg_area, overflow_arg_area_p);
 
   // AMD64-ABI 3.5.7p5: Step 11. Return the fetched type.
-  return Address(Res, CharUnits::fromQuantity(Align));
+  return Address(Res, Align);
 }
 
 Address X86_64ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                  QualType Ty) const {
   // Assume that va_list type is correct; should be pointer to LLVM type:
   // struct {
   //   i32 gp_offset;
   //   i32 fp_offset;
if (!isIndirect) {
       auto TypeInfo = CGF.getContext().getTypeInfoInChars(Ty);
       Size = TypeInfo.first.RoundUpToAlignment(OverflowAreaAlign);
     } else {
       Size = CGF.getPointerSize();
     }
 
     Address OverflowAreaAddr =
       Builder.CreateStructGEP(VAList, 3, CharUnits::fromQuantity(4));
-    Address OverflowArea(Builder.CreateLoad(OverflowAreaAddr),
+    Address OverflowArea(Builder.CreateLoad(OverflowAreaAddr, "argp.cur"),
                          OverflowAreaAlign);
+    // Round up address of argument to alignment
+    CharUnits Align = CGF.getContext().getTypeAlignInChars(Ty);
+    if (Align > OverflowAreaAlign) {
+      llvm::Value *Ptr = OverflowArea.getPointer();
+      OverflowArea = Address(emitRoundPointerUpToAlignment(CGF, Ptr, Align),
+                                                           Align);
+    }
  
-    // The current address is the address of the varargs element.
-    // FIXME: do we not need to round up to alignment?
     MemAddr = Builder.CreateElementBitCast(OverflowArea, DirectTy);
 
     // Increase the overflow area.
     OverflowArea = Builder.CreateConstInBoundsByteGEP(OverflowArea, Size);
     Builder.CreateStore(OverflowArea.getPointer(), OverflowAreaAddr);
     CGF.EmitBranch(Cont);
   }
 
   CGF.EmitBlock(Cont);
 
   // Merge the cases with a phi.
   Address Result = emitMergePHI(CGF, RegAddr, UsingRegs, MemAddr, UsingOverflow,
                                 "vaarg.addr");
 
   // Load the pointer if the argument was passed indirectly.
   if (isIndirect) {
     Result = Address(Builder.CreateLoad(Result, "aggr"),

test/CodeGen/ppc-varargs-struct.c

 // CHECK-PPC-NEXT:  [[REGADDR:%.+]] = bitcast i8* [[RAW_REGADDR]] to %struct.x**
 // CHECK-PPC-NEXT:  [[USED_GPR:%[0-9]+]] = add i8 [[GPR]], 1
 // CHECK-PPC-NEXT:  store i8 [[USED_GPR]], i8* [[GPRPTR]], align 4
 // CHECK-PPC-NEXT:  br label %[[CONT:[a-z0-9]+]]
 //
 // CHECK-PPC:[[USING_OVERFLOW]]
 // CHECK-PPC-NEXT:  [[OVERFLOW_AREA_P:%[0-9]+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* [[ARRAYDECAY]], i32 0, i32 3
 // CHECK-PPC-NEXT:  [[OVERFLOW_AREA:%.+]] = load i8*, i8** [[OVERFLOW_AREA_P]], align 4
-// CHECK-PPC-NEXT:  [[MEMADDR:%.+]] = bitcast i8* [[OVERFLOW_AREA]] to %struct.x**
-// CHECK-PPC-NEXT:  [[NEW_OVERFLOW_AREA:%[0-9]+]] = getelementptr inbounds i8, i8* [[OVERFLOW_AREA]], i32 4
-// CHECK-PPC-NEXT:  store i8* [[NEW_OVERFLOW_AREA]], i8** [[OVERFLOW_AREA_P]]
+// CHECK-PPC-NEXT:  %{{[0-9]+}} =  ptrtoint i8* %argp.cur to i32
+// CHECK-PPC-NEXT:  %{{[0-9]+}} = add i32 %{{[0-9]+}}, 7
+// CHECK-PPC-NEXT:  %{{[0-9]+}} = and i32 %{{[0-9]+}}, -8
+// CHECK-PPC-NEXT:  %argp.cur.aligned = inttoptr i32 %{{[0-9]+}} to i8*
+// CHECK-PPC-NEXT:  [[MEMADDR:%.+]] = bitcast i8* %argp.cur.aligned to %struct.x**
+// CHECK-PPC-NEXT:  [[NEW_OVERFLOW_AREA:%[0-9]+]] = getelementptr inbounds i8, i8* %argp.cur.aligned, i32 4
+// CHECK-PPC-NEXT:  store i8* [[NEW_OVERFLOW_AREA:%[0-9]+]], i8** [[OVERFLOW_AREA_P]], align 4
 // CHECK-PPC-NEXT:  br label %[[CONT]]
 //
 // CHECK-PPC:[[CONT]]
 // CHECK-PPC-NEXT:  [[VAARG_ADDR:%[a-z.0-9]+]] = phi %struct.x** [ [[REGADDR]], %[[USING_REGS]] ], [ [[MEMADDR]], %[[USING_OVERFLOW]] ]
 // CHECK-PPC-NEXT:  [[AGGR:%[a-z0-9]+]] = load %struct.x*, %struct.x** [[VAARG_ADDR]]
 // CHECK-PPC-NEXT:  [[DEST:%[0-9]+]] = bitcast %struct.x* %t to i8*
 // CHECK-PPC-NEXT:  [[SRC:%.+]] = bitcast %struct.x* [[AGGR]] to i8*
 // CHECK-PPC-NEXT:  call void @llvm.memcpy.p0i8.p0i8.i32(i8* [[DEST]], i8* [[SRC]], i32 16, i32 8, i1 false)

test/CodeGen/x86_64-arguments.c

 
 extern void func42(SA s);
 void func43(SA s) {
   func42(s);
 }
 
 // CHECK-LABEL: define i32 @f44
 // CHECK: ptrtoint
-// CHECK-NEXT: and {{.*}}, -32
+// CHECK-NEXT: add i64 %{{[0-9]+}}, 31
+// CHECK-NEXT: and i64 %{{[0-9]+}}, -32
 // CHECK-NEXT: inttoptr
 typedef int T44 __attribute((vector_size(32)));
 struct s44 { T44 x; int y; };
 int f44(int i, ...) {
   __builtin_va_list ap;
   __builtin_va_start(ap, i);
   struct s44 s = __builtin_va_arg(ap, struct s44);
   __builtin_va_end(ap);