[LLVMdev] RFC: Store alignment should be LValue alignment, not source alignment

[Evan Cheng]

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.