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. :slight_smile:

Evan

Index: gcc/llvm-convert.cpp

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. :slight_smile:

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.

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. :slight_smile:

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