Please review the attached patch.
I added comments and changed the type of ABIArgInfo's padding to llvm::Type*.
________________________________________
From: Eli Friedman [[email protected]]
Sent: Wednesday, January 04, 2012 4:10 PM
To: Hatanaka, Akira
Cc: [email protected]
Subject: Re: [cfe-commits] r143596 - /cfe/trunk/lib/CodeGen/TargetInfo.cpp
On Wed, Jan 4, 2012 at 11:21 AM, Hatanaka, Akira <[email protected]> wrote:
> So I finally came up with a way to fix the alignment issue.
> Please review the attached patch & test case.
>
> The idea is to add a parameter to ABIArgInfo::getDirect that is used to set
> the size of a padding inserted before a byval aggregate. The test case shows
> padding is inserted before the first element of a 16-byte aligned byval
> structure when the first element is not aligned to a 16-byte boundary.
The one conceptual issue that I have with this patch is that measuring
the padding in bytes isn't very general or intuitive; how many
registers is 8 bytes? What if some target needs to insert a float of
padding? I think if target-specific code decides it needs an i64 of
padding, it should put the llvm::Type "i64" into ABIArgInfo, etc.
Some more comments for the target-independent changes would be nice.
Otherwise, mechanically, the patch looks fine.
-Eli
Index: lib/CodeGen/ABIInfo.h
===================================================================
--- lib/CodeGen/ABIInfo.h (revision 147607)
+++ lib/CodeGen/ABIInfo.h (working copy)
@@ -42,7 +42,8 @@
/// type, or by coercing to another specified type stored in
/// 'CoerceToType'). If an offset is specified (in UIntData), then the
/// argument passed is offset by some number of bytes in the memory
- /// representation.
+ /// representation. A dummy argument is emitted before the real argument
+ /// if the specified type stored in "PaddingType" is not zero.
Direct,
/// Extend - Valid only for integer argument types. Same as 'direct'
@@ -68,20 +69,22 @@
private:
Kind TheKind;
- llvm::Type *TypeData;
+ llvm::Type *TypeData, *PaddingType;
unsigned UIntData;
bool BoolData0;
bool BoolData1;
- ABIArgInfo(Kind K, llvm::Type *TD=0,
- unsigned UI=0, bool B0 = false, bool B1 = false)
- : TheKind(K), TypeData(TD), UIntData(UI), BoolData0(B0), BoolData1(B1) {}
+ ABIArgInfo(Kind K, llvm::Type *TD=0, unsigned UI=0,
+ bool B0 = false, bool B1 = false, llvm::Type* P = 0)
+ : TheKind(K), TypeData(TD), PaddingType(P), UIntData(UI), BoolData0(B0),
+ BoolData1(B1) {}
public:
ABIArgInfo() : TheKind(Direct), TypeData(0), UIntData(0) {}
- static ABIArgInfo getDirect(llvm::Type *T = 0, unsigned Offset = 0) {
- return ABIArgInfo(Direct, T, Offset);
+ static ABIArgInfo getDirect(llvm::Type *T = 0, unsigned Offset = 0,
+ llvm::Type *Padding = 0) {
+ return ABIArgInfo(Direct, T, Offset, false, false, Padding);
}
static ABIArgInfo getExtend(llvm::Type *T = 0) {
return ABIArgInfo(Extend, T, 0);
@@ -113,6 +116,12 @@
assert((isDirect() || isExtend()) && "Not a direct or extend kind");
return UIntData;
}
+
+ llvm::Type *getPaddingType() const {
+ assert(isDirect() && "Not a direct kind");
+ return PaddingType;
+ }
+
llvm::Type *getCoerceToType() const {
assert(canHaveCoerceToType() && "Invalid kind!");
return TypeData;
Index: lib/CodeGen/TargetInfo.cpp
===================================================================
--- lib/CodeGen/TargetInfo.cpp (revision 147607)
+++ lib/CodeGen/TargetInfo.cpp (working copy)
@@ -3047,7 +3047,7 @@
ABIInfo(CGT), IsO32(_IsO32), MinABIStackAlignInBytes(IsO32 ? 4 : 8) {}
ABIArgInfo classifyReturnType(QualType RetTy) const;
- ABIArgInfo classifyArgumentType(QualType RetTy) const;
+ ABIArgInfo classifyArgumentType(QualType RetTy, uint64_t &Offset) const;
virtual void computeInfo(CGFunctionInfo &FI) const;
virtual llvm::Value *EmitVAArg(llvm::Value *VAListAddr, QualType Ty,
CodeGenFunction &CGF) const;
@@ -3132,28 +3132,41 @@
return llvm::StructType::get(getVMContext(), ArgList);
}
-ABIArgInfo MipsABIInfo::classifyArgumentType(QualType Ty) const {
+ABIArgInfo
+MipsABIInfo::classifyArgumentType(QualType Ty, uint64_t &Offset) const {
if (isAggregateTypeForABI(Ty)) {
// Ignore empty aggregates.
- if (getContext().getTypeSize(Ty) == 0)
+ uint64_t TySize = getContext().getTypeSize(Ty);
+ if (TySize == 0)
return ABIArgInfo::getIgnore();
// Records with non trivial destructors/constructors should not be passed
// by value.
- if (isRecordWithNonTrivialDestructorOrCopyConstructor(Ty))
+ if (isRecordWithNonTrivialDestructorOrCopyConstructor(Ty)) {
+ Offset += 8;
return ABIArgInfo::getIndirect(0, /*ByVal=*/false);
+ }
- llvm::Type *ResType;
- if ((ResType = HandleStructTy(Ty)))
- return ABIArgInfo::getDirect(ResType);
-
- return ABIArgInfo::getIndirect(0);
+ // If we have reached here, aggregates are passed either indirectly via a
+ // byval pointer or directly by coercing to another structure type. In the
+ // latter case, padding is inserted if the offset of the aggregate is
+ // unaligned.
+ llvm::Type *ResType = HandleStructTy(Ty);
+ uint64_t Align = getContext().getTypeAlign(Ty) / 8;
+ assert(Align <= 16 && "Alignment larger than 16 not handled.");
+ llvm::Type *PaddingTy = (ResType && Align == 16 && Offset & 0xf) ?
+ llvm::IntegerType::get(getVMContext(), 64) : 0;
+ Offset = llvm::RoundUpToAlignment(Offset, std::max(Align, (uint64_t)8));
+ Offset += llvm::RoundUpToAlignment(TySize, 8);
+ return ResType ? ABIArgInfo::getDirect(ResType, 0, PaddingTy) :
+ ABIArgInfo::getIndirect(0);
}
// Treat an enum type as its underlying type.
if (const EnumType *EnumTy = Ty->getAs<EnumType>())
Ty = EnumTy->getDecl()->getIntegerType();
+ Offset += 8;
return (Ty->isPromotableIntegerType() ?
ABIArgInfo::getExtend() : ABIArgInfo::getDirect());
}
@@ -3230,9 +3243,10 @@
void MipsABIInfo::computeInfo(CGFunctionInfo &FI) const {
FI.getReturnInfo() = classifyReturnType(FI.getReturnType());
+ uint64_t Offset = 0;
for (CGFunctionInfo::arg_iterator it = FI.arg_begin(), ie = FI.arg_end();
it != ie; ++it)
- it->info = classifyArgumentType(it->type);
+ it->info = classifyArgumentType(it->type, Offset);
}
llvm::Value* MipsABIInfo::EmitVAArg(llvm::Value *VAListAddr, QualType Ty,
Index: lib/CodeGen/CGCall.cpp
===================================================================
--- lib/CodeGen/CGCall.cpp (revision 147607)
+++ lib/CodeGen/CGCall.cpp (working copy)
@@ -691,6 +691,9 @@
// generally likes scalar values better than FCAs.
llvm::Type *argType = argAI.getCoerceToType();
if (llvm::StructType *st = dyn_cast<llvm::StructType>(argType)) {
+ // Insert a padding type to ensure proper alignment.
+ if (llvm::Type *PaddingType = argAI.getPaddingType())
+ argTypes.push_back(PaddingType);
for (unsigned i = 0, e = st->getNumElements(); i != e; ++i)
argTypes.push_back(st->getElementType(i));
} else {
@@ -842,7 +845,8 @@
if (llvm::StructType *STy =
dyn_cast<llvm::StructType>(AI.getCoerceToType()))
- Index += STy->getNumElements()-1; // 1 will be added below.
+ // 1 will be added below.
+ Index += STy->getNumElements() + (AI.getPaddingType() != 0) - 1;
break;
case ABIArgInfo::Indirect:
@@ -1030,7 +1034,9 @@
if (llvm::StructType *STy =
dyn_cast<llvm::StructType>(ArgI.getCoerceToType())) {
Ptr = Builder.CreateBitCast(Ptr, llvm::PointerType::getUnqual(STy));
-
+ // Skip the dummy padding argument.
+ if (ArgI.getPaddingType())
+ ++AI;
for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
assert(AI != Fn->arg_end() && "Argument mismatch!");
AI->setName(Arg->getName() + ".coerce" + Twine(i));
@@ -1705,6 +1711,11 @@
dyn_cast<llvm::StructType>(ArgInfo.getCoerceToType())) {
SrcPtr = Builder.CreateBitCast(SrcPtr,
llvm::PointerType::getUnqual(STy));
+ // Insert a padding argument to ensure proper alignment.
+ if (llvm::Type *PaddingType = ArgInfo.getPaddingType()) {
+ Args.push_back(llvm::UndefValue::get(PaddingType));
+ ++IRArgNo;
+ }
for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {
llvm::Value *EltPtr = Builder.CreateConstGEP2_32(SrcPtr, 0, i);
llvm::LoadInst *LI = Builder.CreateLoad(EltPtr);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
