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

[Duncan Sands]

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.