Evan Cheng
2009-Jan-09 19:05 UTC
[LLVMdev] RFC: Store alignment should be LValue alignment, not source alignment
Hi all,
Please review this patch. It's fixing PR3232 comment #8. Function bar
from 2008-03-24-BitFiled-And-Alloca.c compiles to:
%struct.Key = type { { i32, i32 } }
...
define i32 @bar(i64 %key_token2) nounwind {
entry:
%key_token2_addr = alloca i64 ; <i64*> [#uses=2]
%retval = alloca i32 ; <i32*> [#uses=2]
%iospec = alloca %struct.Key ; <%struct.Key*>
[#uses=3]
%ret = alloca i32 ; <i32*> [#uses=2]
%0 = alloca i32 ; <i32*> [#uses=2]
%"alloca point" = bitcast i32 0 to i32 ; <i32>
[#uses=0]
store i64 %key_token2, i64* %key_token2_addr
%1 = getelementptr %struct.Key* %iospec, i32 0, i32
0 ; <{ i32, i32 }*> [#uses=2]
%2 = getelementptr { i32, i32 }* %1, i32 0, i32 0 ;
<i32*> [#uses=1]
store i32 0, i32* %2, align 4
%3 = getelementptr { i32, i32 }* %1, i32 0, i32
1 ; <i32*> [#uses=1]
store i32 0, i32* %3, align 4
%4 = getelementptr %struct.Key* %iospec, i32 0, i32
0 ; <{ i32, i32 }*> [#uses=1]
%5 = bitcast { i32, i32 }* %4 to i64* ; <i64*>
[#uses=1]
%6 = load i64* %key_token2_addr, align 8 ;
<i64> [#uses=1]
store i64 %6, i64* %5, align 8
...
The store alignment 8 is wrong. The address iospec has 4-byte
alignment. The problem is llvm-gcc TreeToLLVM::EmitMODIFY_EXPR:
LValue LV = EmitLV(lhs);
bool isVolatile = TREE_THIS_VOLATILE(lhs);
unsigned Alignment = expr_align(exp) / 8
It's using the alignment of the expression, rather than the memory
object of LValue.
The patch saves the alignment of the memory object in LValue returned
by EmitLV(). Please review it carefully as I am not entirely
comfortable hacking on llvm-gcc. :-)
Evan
Index: gcc/llvm-convert.cpp
==================================================================---
gcc/llvm-convert.cpp (revision 61984)
+++ gcc/llvm-convert.cpp (working copy)
@@ -1150,9 +1150,18 @@ LValue TreeToLLVM::EmitLV(tree exp) {
case IMAGPART_EXPR: return EmitLV_XXXXPART_EXPR(exp, 1);
// Constants.
- case LABEL_DECL: return TreeConstantToLLVM::EmitLV_LABEL_DECL(exp);
- case COMPLEX_CST: return
LValue(TreeConstantToLLVM::EmitLV_COMPLEX_CST(exp));
- case STRING_CST: return
LValue(TreeConstantToLLVM::EmitLV_STRING_CST(exp));
+ case LABEL_DECL: {
+ Value *Ptr = TreeConstantToLLVM::EmitLV_LABEL_DECL(exp);
+ return LValue(Ptr, DECL_ALIGN(exp) / 8);
+ }
+ case COMPLEX_CST: {
+ Value *Ptr = TreeConstantToLLVM::EmitLV_COMPLEX_CST(exp);
+ return LValue(Ptr, TYPE_ALIGN(TREE_TYPE(exp)) / 8);
+ }
+ case STRING_CST: {
+ Value *Ptr = TreeConstantToLLVM::EmitLV_STRING_CST(exp);
+ return LValue(Ptr, TYPE_ALIGN(TREE_TYPE(exp)) / 8);
+ }
// Type Conversion.
case VIEW_CONVERT_EXPR: return EmitLV_VIEW_CONVERT_EXPR(exp);
@@ -1165,9 +1174,11 @@ LValue TreeToLLVM::EmitLV(tree exp) {
case WITH_SIZE_EXPR:
// The address is the address of the operand.
return EmitLV(TREE_OPERAND(exp, 0));
- case INDIRECT_REF:
+ case INDIRECT_REF: {
// The lvalue is just the address.
- return Emit(TREE_OPERAND(exp, 0), 0);
+ tree Op = TREE_OPERAND(exp, 0);
+ return LValue(Emit(Op, 0), expr_align(Op) / 8);
+ }
}
}
@@ -2290,7 +2301,7 @@ Value *TreeToLLVM::EmitLoadOfLValue(tree
LValue LV = EmitLV(exp);
bool isVolatile = TREE_THIS_VOLATILE(exp);
const Type *Ty = ConvertType(TREE_TYPE(exp));
- unsigned Alignment = expr_align(exp) / 8;
+ unsigned Alignment = LV.getAlignment();
if (TREE_CODE(exp) == COMPONENT_REF)
if (const StructType *STy
dyn_cast<StructType>(ConvertType(TREE_TYPE(TREE_OPERAND(exp,
0)))))
@@ -2963,7 +2974,7 @@ Value *TreeToLLVM::EmitMODIFY_EXPR(tree
LValue LV = EmitLV(lhs);
bool isVolatile = TREE_THIS_VOLATILE(lhs);
- unsigned Alignment = expr_align(lhs) / 8;
+ unsigned Alignment = LV.getAlignment();
if (TREE_CODE(lhs) == COMPONENT_REF)
if (const StructType *STy
dyn_cast<StructType>(ConvertType(TREE_TYPE(TREE_OPERAND(lhs,
0)))))
@@ -3157,7 +3168,7 @@ Value *TreeToLLVM::EmitVIEW_CONVERT_EXPR
LValue LV = EmitLV(Op);
assert(!LV.isBitfield() && "Expected an aggregate
operand!");
bool isVolatile = TREE_THIS_VOLATILE(Op);
- unsigned Alignment = expr_align(Op) / 8;
+ unsigned Alignment = LV.getAlignment();
EmitAggregateCopy(Target, MemRef(LV.Ptr, Alignment, isVolatile),
TREE_TYPE(exp));
@@ -5885,9 +5896,10 @@ LValue TreeToLLVM::EmitLV_DECL(tree exp)
Value *Decl = DECL_LLVM(exp);
if (Decl == 0) {
if (errorcount || sorrycount) {
- const PointerType *Ty -
PointerType::getUnqual(ConvertType(TREE_TYPE(exp)));
- return ConstantPointerNull::get(Ty);
+ const Type *Ty = ConvertType(TREE_TYPE(exp));
+ const PointerType *PTy = PointerType::getUnqual(Ty);
+ LValue LV(ConstantPointerNull::get(PTy),
TD.getABITypeAlignment(Ty));
+ return LV;
}
assert(0 && "INTERNAL ERROR: Referencing decl that hasn't
been
laid out");
abort();
@@ -5924,7 +5936,13 @@ LValue TreeToLLVM::EmitLV_DECL(tree exp)
// type void.
if (Ty == Type::VoidTy) Ty = StructType::get(NULL, NULL);
const PointerType *PTy = PointerType::getUnqual(Ty);
- return BitCastToType(Decl, PTy);
+ unsigned Alignment = Ty->isSized() ? TD.getABITypeAlignment(Ty) : 1;
+ if (DECL_ALIGN_UNIT(exp)) {
+ if (DECL_USER_ALIGN(exp) || Alignment <
(unsigned)DECL_ALIGN_UNIT(exp))
+ Alignment = DECL_ALIGN_UNIT(exp);
+ }
+
+ return LValue(BitCastToType(Decl, PTy), Alignment);
}
LValue TreeToLLVM::EmitLV_ARRAY_REF(tree exp) {
@@ -5932,22 +5950,23 @@ LValue TreeToLLVM::EmitLV_ARRAY_REF(tree
// of ElementTy in the case of ARRAY_RANGE_REF.
tree Array = TREE_OPERAND(exp, 0);
- tree ArrayType = TREE_TYPE(Array);
+ tree ArrayTreeType = TREE_TYPE(Array);
tree Index = TREE_OPERAND(exp, 1);
tree IndexType = TREE_TYPE(Index);
- tree ElementType = TREE_TYPE(ArrayType);
+ tree ElementType = TREE_TYPE(ArrayTreeType);
- assert((TREE_CODE (ArrayType) == ARRAY_TYPE ||
- TREE_CODE (ArrayType) == POINTER_TYPE ||
- TREE_CODE (ArrayType) == REFERENCE_TYPE ||
- TREE_CODE (ArrayType) == BLOCK_POINTER_TYPE) &&
+ assert((TREE_CODE (ArrayTreeType) == ARRAY_TYPE ||
+ TREE_CODE (ArrayTreeType) == POINTER_TYPE ||
+ TREE_CODE (ArrayTreeType) == REFERENCE_TYPE ||
+ TREE_CODE (ArrayTreeType) == BLOCK_POINTER_TYPE) &&
"Unknown ARRAY_REF!");
// As an LLVM extension, we allow ARRAY_REF with a pointer as the
first
// operand. This construct maps directly to a getelementptr
instruction.
Value *ArrayAddr;
+ unsigned ArrayAlign;
- if (TREE_CODE(ArrayType) == ARRAY_TYPE) {
+ if (TREE_CODE(ArrayTreeType) == ARRAY_TYPE) {
// First subtract the lower bound, if any, in the type of the
index.
tree LowerBound = array_ref_low_bound(exp);
if (!integer_zerop(LowerBound))
@@ -5956,8 +5975,10 @@ LValue TreeToLLVM::EmitLV_ARRAY_REF(tree
LValue ArrayAddrLV = EmitLV(Array);
assert(!ArrayAddrLV.isBitfield() && "Arrays cannot be
bitfields!");
ArrayAddr = ArrayAddrLV.Ptr;
+ ArrayAlign = ArrayAddrLV.Alignment;
} else {
ArrayAddr = Emit(Array, 0);
+ ArrayAlign = expr_align(ArrayTreeType) / 8;
}
Value *IndexVal = Emit(Index, 0);
@@ -5971,20 +5992,27 @@ LValue TreeToLLVM::EmitLV_ARRAY_REF(tree
IndexVal = CastToSIntType(IndexVal, IntPtrTy);
// If this is an index into an LLVM array, codegen as a GEP.
- if (isArrayCompatible(ArrayType)) {
+ if (isArrayCompatible(ArrayTreeType)) {
Value *Idxs[2] = { ConstantInt::get(Type::Int32Ty, 0), IndexVal };
Value *Ptr = Builder.CreateGEP(ArrayAddr, Idxs, Idxs + 2);
- return BitCastToType(Ptr,
-
PointerType::getUnqual(ConvertType(TREE_TYPE(exp))));
+ const Type *ATy = cast<PointerType>(ArrayAddr->getType())-
>getElementType();
+ const Type *ElementTy = cast<ArrayType>(ATy)->getElementType();
+ unsigned Alignment = MinAlign(ArrayAlign,
TD.getABITypeSize(ElementTy));
+ return LValue(BitCastToType(Ptr,
+
PointerType::getUnqual(ConvertType(TREE_TYPE(exp)))),
+ Alignment);
}
// If we are indexing over a fixed-size type, just use a GEP.
- if (isSequentialCompatible(ArrayType)) {
- const Type *PtrElementTy =
PointerType::getUnqual(ConvertType(ElementType));
+ if (isSequentialCompatible(ArrayTreeType)) {
+ const Type *ElementTy = ConvertType(ElementType);
+ const Type *PtrElementTy = PointerType::getUnqual(ElementTy);
ArrayAddr = BitCastToType(ArrayAddr, PtrElementTy);
Value *Ptr = Builder.CreateGEP(ArrayAddr, IndexVal);
- return BitCastToType(Ptr,
-
PointerType::getUnqual(ConvertType(TREE_TYPE(exp))));
+ unsigned Alignment = MinAlign(ArrayAlign,
TD.getABITypeAlignment(ElementTy));
+ return LValue(BitCastToType(Ptr,
+
PointerType::getUnqual(ConvertType(TREE_TYPE(exp)))),
+ Alignment);
}
// Otherwise, just do raw, low-level pointer arithmetic. FIXME:
this could be
@@ -5992,14 +6020,21 @@ LValue TreeToLLVM::EmitLV_ARRAY_REF(tree
// float foo(int w, float A[][w], int g) { return A[g][0]; }
ArrayAddr = BitCastToType(ArrayAddr,
PointerType::getUnqual(Type::Int8Ty));
- if (VOID_TYPE_P(TREE_TYPE(ArrayType)))
- return Builder.CreateGEP(ArrayAddr, IndexVal);
+ if (VOID_TYPE_P(TREE_TYPE(ArrayTreeType))) {
+ unsigned Alignment = MinAlign(ArrayAlign,
+
TD.getABITypeAlignment(Type::Int8Ty));
+ return LValue(Builder.CreateGEP(ArrayAddr, IndexVal), Alignment);
+ }
Value *TypeSize = Emit(array_ref_element_size(exp), 0);
TypeSize = CastToUIntType(TypeSize, IntPtrTy);
IndexVal = Builder.CreateMul(IndexVal, TypeSize);
Value *Ptr = Builder.CreateGEP(ArrayAddr, IndexVal);
- return
BitCastToType(Ptr,PointerType::getUnqual(ConvertType(TREE_TYPE(exp))));
+ unsigned Alignment = MinAlign(ArrayAlign,
+ cast<ConstantInt>(IndexVal)-
>getZExtValue());
+ return LValue(BitCastToType(Ptr,
+
PointerType::getUnqual(ConvertType(TREE_TYPE(exp)))),
+ Alignment);
}
/// getFieldOffsetInBits - Return the offset (in bits) of a
FIELD_DECL in a
@@ -6028,8 +6063,9 @@ static unsigned getComponentRefOffsetInB
LValue TreeToLLVM::EmitLV_COMPONENT_REF(tree exp) {
LValue StructAddrLV = EmitLV(TREE_OPERAND(exp, 0));
- tree FieldDecl = TREE_OPERAND(exp, 1);
-
+ tree FieldDecl = TREE_OPERAND(exp, 1);
+ unsigned LVAlign = DECL_PACKED(FieldDecl) ? 1 :
StructAddrLV.Alignment;
+
assert((TREE_CODE(DECL_CONTEXT(FieldDecl)) == RECORD_TYPE ||
TREE_CODE(DECL_CONTEXT(FieldDecl)) == UNION_TYPE ||
TREE_CODE(DECL_CONTEXT(FieldDecl)) == QUAL_UNION_TYPE));
@@ -6064,7 +6100,9 @@ LValue TreeToLLVM::EmitLV_COMPONENT_REF(
// the offset from BitStart.
if (MemberIndex) {
const StructLayout *SL =
TD.getStructLayout(cast<StructType>(StructTy));
- BitStart -= SL->getElementOffset(MemberIndex) * 8;
+ unsigned Offset = SL->getElementOffset(MemberIndex);
+ BitStart -= Offset * 8;
+ LVAlign = MinAlign(LVAlign, Offset);
}
// If the FIELD_DECL has an annotate attribute on it, emit it.
@@ -6130,7 +6168,7 @@ LValue TreeToLLVM::EmitLV_COMPONENT_REF(
if (AnnotateAttr)
AnnotateAttr = lookup_attribute("annotate", AnnotateAttr);
}
- }
+ }
} else {
Value *Offset = Emit(field_offset, 0);
@@ -6150,6 +6188,7 @@ LValue TreeToLLVM::EmitLV_COMPONENT_REF(
Offset = Builder.CreateAdd(Offset,
ConstantInt::get(Offset->getType(), ByteOffset));
BitStart -= ByteOffset*8;
+ LVAlign = MinAlign(LVAlign, ByteOffset);
}
Value *Ptr = CastToType(Instruction::PtrToInt, StructAddrLV.Ptr,
@@ -6221,6 +6260,7 @@ LValue TreeToLLVM::EmitLV_COMPONENT_REF(
// Compute the byte offset, and add it to the pointer.
unsigned ByteOffset = NumAlignmentUnits*ByteAlignment;
+ LVAlign = MinAlign(LVAlign, ByteOffset);
Constant *Offset = ConstantInt::get(TD.getIntPtrType(),
ByteOffset);
FieldPtr = CastToType(Instruction::PtrToInt, FieldPtr,
@@ -6242,17 +6282,18 @@ LValue TreeToLLVM::EmitLV_COMPONENT_REF(
// Okay, everything is good. Return this as a bitfield if we
can't
// return it as a normal l-value. (e.g. "struct X { int X :
32 };" ).
+ // Conservatively return LValue with alignment 1.
if (BitfieldSize != LLVMValueBitSize || BitStart != 0)
- return LValue(FieldPtr, BitStart, BitfieldSize);
+ return LValue(FieldPtr, 1, BitStart, BitfieldSize);
} else {
// Make sure we return a pointer to the right type.
- FieldPtr = BitCastToType(FieldPtr,
-
PointerType::getUnqual(ConvertType(TREE_TYPE(exp))));
+ const Type *EltTy = ConvertType(TREE_TYPE(exp));
+ FieldPtr = BitCastToType(FieldPtr, PointerType::getUnqual(EltTy));
}
assert(BitStart == 0 &&
"It's a bitfield reference or we didn't get to the
field!");
- return LValue(FieldPtr);
+ return LValue(FieldPtr, LVAlign);
}
LValue TreeToLLVM::EmitLV_BIT_FIELD_REF(tree exp) {
@@ -6284,17 +6325,27 @@ LValue TreeToLLVM::EmitLV_BIT_FIELD_REF(
}
// If this is referring to the whole field, return the whole thing.
- if (BitStart == 0 && BitSize == ValueSizeInBits)
- return LValue(BitCastToType(Ptr.Ptr,
PointerType::getUnqual(ValTy)));
+ if (BitStart == 0 && BitSize == ValueSizeInBits) {
+ return LValue(BitCastToType(Ptr.Ptr,
PointerType::getUnqual(ValTy)),
+ Ptr.Alignment);
+ }
- return LValue(BitCastToType(Ptr.Ptr,
PointerType::getUnqual(ValTy)), BitStart,
- BitSize);
+ return LValue(BitCastToType(Ptr.Ptr,
PointerType::getUnqual(ValTy)), 1,
+ BitStart, BitSize);
}
LValue TreeToLLVM::EmitLV_XXXXPART_EXPR(tree exp, unsigned Idx) {
LValue Ptr = EmitLV(TREE_OPERAND(exp, 0));
- assert(!Ptr.isBitfield() && "BIT_FIELD_REF operands cannot be
bitfields!");
- return LValue(Builder.CreateStructGEP(Ptr.Ptr, Idx));
+ assert(!Ptr.isBitfield() &&
+ "REALPART_EXPR / IMAGPART_EXPR operands cannot be
bitfields!");
+ unsigned Alignment;
+ if (Idx == 0)
+ // REALPART alignment is same as the complex operand.
+ Alignment = Ptr.Alignment;
+ else
+ // IMAGPART alignment = MinAlign(Ptr.Alignment, sizeof field);
+ Alignment = MinAlign(Ptr.Alignment, TD.getABITypeSize(Ptr.Ptr-
>getType()));
+ return LValue(Builder.CreateStructGEP(Ptr.Ptr, Idx), Alignment);
}
LValue TreeToLLVM::EmitLV_VIEW_CONVERT_EXPR(tree exp) {
@@ -6310,24 +6361,30 @@ LValue TreeToLLVM::EmitLV_VIEW_CONVERT_E
} else {
// If the input is a scalar, emit to a temporary.
Value *Dest = CreateTemporary(ConvertType(TREE_TYPE(Op)));
- Builder.CreateStore(Emit(Op, 0), Dest);
+ StoreInst *S = Builder.CreateStore(Emit(Op, 0), Dest);
// The type is the type of the expression.
Dest = BitCastToType(Dest,
PointerType::getUnqual(ConvertType(TREE_TYPE(exp))));
- return LValue(Dest);
+ return LValue(Dest, S->getAlignment());
}
}
LValue TreeToLLVM::EmitLV_EXC_PTR_EXPR(tree exp) {
CreateExceptionValues();
// Cast the address pointer to the expected type.
- return BitCastToType(ExceptionValue,
-
PointerType::getUnqual(ConvertType(TREE_TYPE(exp))));
+ unsigned Alignment =
TD.getABITypeAlignment(cast<PointerType>(ExceptionValue->
+ getType())-
>getElementType());
+ return LValue(BitCastToType(ExceptionValue,
+
PointerType::getUnqual(ConvertType(TREE_TYPE(exp)))),
+ Alignment);
}
LValue TreeToLLVM::EmitLV_FILTER_EXPR(tree exp) {
CreateExceptionValues();
- return ExceptionSelectorValue;
+ unsigned Alignment +
TD.getABITypeAlignment(cast<PointerType>(ExceptionSelectorValue->
+ getType())-
>getElementType());
+ return LValue(ExceptionSelectorValue, Alignment);
}
//
=
=
=----------------------------------------------------------------------
===//
Index: gcc/llvm-internal.h
==================================================================---
gcc/llvm-internal.h (revision 61984)
+++ gcc/llvm-internal.h (working copy)
@@ -251,23 +251,30 @@ struct MemRef {
};
/// LValue - This struct represents an lvalue in the program. In
particular,
-/// the Ptr member indicates the memory that the lvalue lives in. If
this is
-/// a bitfield reference, BitStart indicates the first bit in the
memory that
-/// is part of the field and BitSize indicates the extent.
+/// the Ptr member indicates the memory that the lvalue lives in.
Alignment
+/// is the alignment of the memory (in bytes).If this is a bitfield
reference,
+/// BitStart indicates the first bit in the memory that is part of
the field
+/// and BitSize indicates the extent.
///
/// "LValue" is intended to be a light-weight object passed around
by-value.
struct LValue {
Value *Ptr;
+ unsigned char Alignment;
unsigned char BitStart;
unsigned char BitSize;
- LValue(Value *P) : Ptr(P), BitStart(255), BitSize(255) {}
- LValue(Value *P, unsigned BSt, unsigned BSi)
- : Ptr(P), BitStart(BSt), BitSize(BSi) {
+ LValue(Value *P, unsigned Align)
+ : Ptr(P), Alignment(Align), BitStart(255), BitSize(255) {}
+ LValue(Value *P, unsigned Align, unsigned BSt, unsigned BSi)
+ : Ptr(P), Alignment(Align), BitStart(BSt), BitSize(BSi) {
assert(BitStart == BSt && BitSize == BSi &&
"Bit values larger than 256?");
+ }
+
+ unsigned getAlignment() const {
+ assert(Alignment && "LValue alignment cannot be zero!");
+ return Alignment;
}
-
bool isBitfield() const { return BitStart != 255; }
};
Duncan Sands
2009-Jan-09 20:33 UTC
[LLVMdev] RFC: Store alignment should be LValue alignment, not source alignment
Hi Evan,> LValue LV = EmitLV(lhs); > bool isVolatile = TREE_THIS_VOLATILE(lhs); > unsigned Alignment = expr_align(exp) / 8 > > It's using the alignment of the expression, rather than the memory > object of LValue.can't you just use expr_align(lhs) instead?> The patch saves the alignment of the memory object in LValue returned > by EmitLV(). Please review it carefully as I am not entirely > comfortable hacking on llvm-gcc. :-)In the long run, LValue and MemRef should be merged, but that can wait until later I suppose.> + case LABEL_DECL: { > + Value *Ptr = TreeConstantToLLVM::EmitLV_LABEL_DECL(exp); > + return LValue(Ptr, DECL_ALIGN(exp) / 8); > + } > + case COMPLEX_CST: { > + Value *Ptr = TreeConstantToLLVM::EmitLV_COMPLEX_CST(exp); > + return LValue(Ptr, TYPE_ALIGN(TREE_TYPE(exp)) / 8); > + } > + case STRING_CST: { > + Value *Ptr = TreeConstantToLLVM::EmitLV_STRING_CST(exp); > + return LValue(Ptr, TYPE_ALIGN(TREE_TYPE(exp)) / 8); > + }These are all equivalent to using expr_align, right?> @@ -2290,7 +2301,7 @@ Value *TreeToLLVM::EmitLoadOfLValue(tree > LValue LV = EmitLV(exp); > bool isVolatile = TREE_THIS_VOLATILE(exp); > const Type *Ty = ConvertType(TREE_TYPE(exp)); > - unsigned Alignment = expr_align(exp) / 8; > + unsigned Alignment = LV.getAlignment();Here expr_align(exp) is correct I think.> @@ -2963,7 +2974,7 @@ Value *TreeToLLVM::EmitMODIFY_EXPR(tree > > LValue LV = EmitLV(lhs); > bool isVolatile = TREE_THIS_VOLATILE(lhs); > - unsigned Alignment = expr_align(lhs) / 8; > + unsigned Alignment = LV.getAlignment();Here I think expr_align(lhs) was correct.> LValue LV = EmitLV(Op); > assert(!LV.isBitfield() && "Expected an aggregate operand!"); > bool isVolatile = TREE_THIS_VOLATILE(Op); > - unsigned Alignment = expr_align(Op) / 8; > + unsigned Alignment = LV.getAlignment();This also looks like it was already ok.> if (errorcount || sorrycount) { > - const PointerType *Ty > - PointerType::getUnqual(ConvertType(TREE_TYPE(exp))); > - return ConstantPointerNull::get(Ty); > + const Type *Ty = ConvertType(TREE_TYPE(exp)); > + const PointerType *PTy = PointerType::getUnqual(Ty); > + LValue LV(ConstantPointerNull::get(PTy), > TD.getABITypeAlignment(Ty)); > + return LV;If the type is opaque or abstract, won't this assert getting the alignment? You might as well use 1 here, since compilation is going to fail anyway.> @@ -5924,7 +5936,13 @@ LValue TreeToLLVM::EmitLV_DECL(tree exp) > // type void. > if (Ty == Type::VoidTy) Ty = StructType::get(NULL, NULL); > const PointerType *PTy = PointerType::getUnqual(Ty); > - return BitCastToType(Decl, PTy); > + unsigned Alignment = Ty->isSized() ? TD.getABITypeAlignment(Ty) : 1;Can't you just use expr_align here? That said, I'm not sure what this case is doing.> + if (DECL_ALIGN_UNIT(exp)) { > + if (DECL_USER_ALIGN(exp) || Alignment < > (unsigned)DECL_ALIGN_UNIT(exp)) > + Alignment = DECL_ALIGN_UNIT(exp); > + } > + > + return LValue(BitCastToType(Decl, PTy), Alignment);Since I don't know what this case handles, I can't comment on this.> LValue ArrayAddrLV = EmitLV(Array); > assert(!ArrayAddrLV.isBitfield() && "Arrays cannot be > bitfields!"); > ArrayAddr = ArrayAddrLV.Ptr; > + ArrayAlign = ArrayAddrLV.Alignment;Couldn't this be expr_align(Array)?> + const Type *ATy = cast<PointerType>(ArrayAddr->getType())- > >getElementType(); > + const Type *ElementTy = cast<ArrayType>(ATy)->getElementType(); > + unsigned Alignment = MinAlign(ArrayAlign, > TD.getABITypeSize(ElementTy));Why these manipulations? These happens several more times below.> @@ -6028,8 +6063,9 @@ static unsigned getComponentRefOffsetInB > > LValue TreeToLLVM::EmitLV_COMPONENT_REF(tree exp) { > LValue StructAddrLV = EmitLV(TREE_OPERAND(exp, 0)); > - tree FieldDecl = TREE_OPERAND(exp, 1); > - > + tree FieldDecl = TREE_OPERAND(exp, 1); > + unsigned LVAlign = DECL_PACKED(FieldDecl) ? 1 : > StructAddrLV.Alignment;Can't this be expr_align(exp)? I'll stop here, because I still don't understand the need for incorporating the alignment into LValue. Ciao, Duncan.
Evan Cheng
2009-Jan-09 21:50 UTC
[LLVMdev] RFC: Store alignment should be LValue alignment, not source alignment
On Jan 9, 2009, at 12:33 PM, Duncan Sands wrote:> Hi Evan, > >> LValue LV = EmitLV(lhs); >> bool isVolatile = TREE_THIS_VOLATILE(lhs); >> unsigned Alignment = expr_align(exp) / 8 >> >> It's using the alignment of the expression, rather than the memory >> object of LValue. > > can't you just use expr_align(lhs) instead?No. My earlier comment is wrong. Under EmitMODIFY_EXPR, it was using expr_align(lhs). But it's still wrong. See this example: extern int bar(unsigned long long key_token2) { ... __attribute__ ((unused)) Key iospec = (Key) key_token2; In the EmitMODIFY_EXPR case, lhs is a component_ref for this: Here is that it looks like: (gdb) call debug_tree(lhs) <component_ref 0x42cd1750 type <integer_type 0x42c0c540 long long unsigned int sizes- gimplified public unsigned DI size <integer_cst 0x42c04930 constant invariant 64> unit size <integer_cst 0x42c04960 constant invariant 8> align 64 symtab 2 alias set -1 precision 64 min <integer_cst 0x42c04a50 0> max <integer_cst 0x42c04a20 18446744073709551615> LLVM: i64> arg 0 <var_decl 0x42cd6850 iospec type <union_type 0x42cd65b0 Key sizes-gimplified type_0 DI size <integer_cst 0x42c04930 64> unit size <integer_cst 0x42c04960 8> align 32 symtab 0 alias set -1 fields <field_decl 0x42cd6310 key_io>> used asm-frame-size 0 DI file t.c line 87 size <integer_cst 0x42c04930 64> unit size <integer_cst 0x42c04960 8> align 32 context <function_decl 0x42cbb600 bar> attributes <tree_list 0x42cc9c80> LLVM: %struct.Key* %iospec> arg 1 <field_decl 0x42cd6540 lkey type <integer_type 0x42c0c540 long long unsigned int> unsigned asm-frame-size 0 DI file t.c line 61 size <integer_cst 0x42c04930 64> unit size <integer_cst 0x42c04960 8> align 32 offset_align 128 offset <integer_cst 0x42c04210 constant invariant 0> bit offset <integer_cst 0x42c04de0 constant invariant 0> context <union_type 0x42cbdc40 _Key>>> Note type of component_ref is i64 with 8-byte alignment. expr_align(lhs) would return 64 here. However, the address of the store is iospec, which is 32-bit aligned.> > >> The patch saves the alignment of the memory object in LValue returned >> by EmitLV(). Please review it carefully as I am not entirely >> comfortable hacking on llvm-gcc. :-) > > In the long run, LValue and MemRef should be merged, but that can > wait until later I suppose. > >> + case LABEL_DECL: { >> + Value *Ptr = TreeConstantToLLVM::EmitLV_LABEL_DECL(exp); >> + return LValue(Ptr, DECL_ALIGN(exp) / 8); >> + } >> + case COMPLEX_CST: { >> + Value *Ptr = TreeConstantToLLVM::EmitLV_COMPLEX_CST(exp); >> + return LValue(Ptr, TYPE_ALIGN(TREE_TYPE(exp)) / 8); >> + } >> + case STRING_CST: { >> + Value *Ptr = TreeConstantToLLVM::EmitLV_STRING_CST(exp); >> + return LValue(Ptr, TYPE_ALIGN(TREE_TYPE(exp)) / 8); >> + } > > These are all equivalent to using expr_align, right?Yes.> > >> @@ -2290,7 +2301,7 @@ Value *TreeToLLVM::EmitLoadOfLValue(tree >> LValue LV = EmitLV(exp); >> bool isVolatile = TREE_THIS_VOLATILE(exp); >> const Type *Ty = ConvertType(TREE_TYPE(exp)); >> - unsigned Alignment = expr_align(exp) / 8; >> + unsigned Alignment = LV.getAlignment(); > > Here expr_align(exp) is correct I think.Not true. The result of the load might be have a generic type. For example, it can be i64 with alignment 64. However, the address can be 64-bit wide but with a different alignment. LLVM load / store instruction alignments are equal to alignment of the memory location, not the value being loaded or stored.> > >> @@ -2963,7 +2974,7 @@ Value *TreeToLLVM::EmitMODIFY_EXPR(tree >> >> LValue LV = EmitLV(lhs); >> bool isVolatile = TREE_THIS_VOLATILE(lhs); >> - unsigned Alignment = expr_align(lhs) / 8; >> + unsigned Alignment = LV.getAlignment(); > > Here I think expr_align(lhs) was correct.No. See before.> > >> LValue LV = EmitLV(Op); >> assert(!LV.isBitfield() && "Expected an aggregate operand!"); >> bool isVolatile = TREE_THIS_VOLATILE(Op); >> - unsigned Alignment = expr_align(Op) / 8; >> + unsigned Alignment = LV.getAlignment(); > > This also looks like it was already ok. > >> if (errorcount || sorrycount) { >> - const PointerType *Ty >> - PointerType::getUnqual(ConvertType(TREE_TYPE(exp))); >> - return ConstantPointerNull::get(Ty); >> + const Type *Ty = ConvertType(TREE_TYPE(exp)); >> + const PointerType *PTy = PointerType::getUnqual(Ty); >> + LValue LV(ConstantPointerNull::get(PTy), >> TD.getABITypeAlignment(Ty)); >> + return LV; > > If the type is opaque or abstract, won't this assert getting the > alignment? You might as well use 1 here, since compilation is > going to fail anyway.Makes sense.> > >> @@ -5924,7 +5936,13 @@ LValue TreeToLLVM::EmitLV_DECL(tree exp) >> // type void. >> if (Ty == Type::VoidTy) Ty = StructType::get(NULL, NULL); >> const PointerType *PTy = PointerType::getUnqual(Ty); >> - return BitCastToType(Decl, PTy); >> + unsigned Alignment = Ty->isSized() ? >> TD.getABITypeAlignment(Ty) : 1; > > Can't you just use expr_align here? That said, I'm not sure what > this case is doing.I am not entirely sure. One thing I noticed is a gcc assigns alignment 64 to a i64 parameter. But TD.getABITypeAlignment will return 32 here (which is correct).> > >> + if (DECL_ALIGN_UNIT(exp)) { >> + if (DECL_USER_ALIGN(exp) || Alignment < >> (unsigned)DECL_ALIGN_UNIT(exp)) >> + Alignment = DECL_ALIGN_UNIT(exp); >> + } >> + >> + return LValue(BitCastToType(Decl, PTy), Alignment); > > Since I don't know what this case handles, I can't comment on this. > >> LValue ArrayAddrLV = EmitLV(Array); >> assert(!ArrayAddrLV.isBitfield() && "Arrays cannot be >> bitfields!"); >> ArrayAddr = ArrayAddrLV.Ptr; >> + ArrayAlign = ArrayAddrLV.Alignment; > > Couldn't this be expr_align(Array)? > >> + const Type *ATy = cast<PointerType>(ArrayAddr->getType())- >>> getElementType(); >> + const Type *ElementTy = cast<ArrayType>(ATy)->getElementType(); >> + unsigned Alignment = MinAlign(ArrayAlign, >> TD.getABITypeSize(ElementTy)); > > Why these manipulations? These happens several more times below.According to Chris, alignment of a vector element is MinAlign(alignof vector, sizeof vector element).> > >> @@ -6028,8 +6063,9 @@ static unsigned getComponentRefOffsetInB >> >> LValue TreeToLLVM::EmitLV_COMPONENT_REF(tree exp) { >> LValue StructAddrLV = EmitLV(TREE_OPERAND(exp, 0)); >> - tree FieldDecl = TREE_OPERAND(exp, 1); >> - >> + tree FieldDecl = TREE_OPERAND(exp, 1); >> + unsigned LVAlign = DECL_PACKED(FieldDecl) ? 1 : >> StructAddrLV.Alignment; > > Can't this be expr_align(exp)?No. It will just return alignment of the expression type.> > > I'll stop here, because I still don't understand the need > for incorporating the alignment into LValue.Hopefully my earlier explanation makes sense on why this is necessary. Evan> > > Ciao, > > Duncan.