[llvm-commits] Stack and register alignment in linux/ppc calls

[Chris Lattner]

On Mar 6, 2007, at 10:03 AM, Nicolas Geoffray wrote:
> This patch corrects arguments passing alignment for linux/ppc calls  
> (ELF ABI).
> It affects LowerFORMAL_ARGUMENTS and LowerCALL of PPCISelLowering.cpp.

Sure, sorry for the delay.  Please add some high-level comments that  
explain what is going on here (what the ABI says).  I would  
eventually like to switch PPC over to using autogenerated callingconv  
code, but I haven't had a chance to finish argument passing.

> @@ -1164,24 +1165,34 @@ static SDOperand LowerFORMAL_ARGUMENTS(S
>      SDOperand ArgVal;
>      bool needsLoad = false;
>      MVT::ValueType ObjectVT = Op.getValue(ArgNo).getValueType();
>      unsigned ObjSize = MVT::getSizeInBits(ObjectVT)/8;
>      unsigned ArgSize = ObjSize;
> +    unsigned Flags = cast<ConstantSDNode>(Op.getOperand(ArgNo+3))- 
> >getValue();
> +    // See if next argument requires stack alignment in ELF
> +    unsigned Expand = (ObjectVT == MVT::f64) || ((ArgNo + 1 < e) &&
> +      (cast<ConstantSDNode>(Op.getOperand(ArgNo+4))->getValue() &  
> (1 << 27)) &&
> +      (!(Flags & (1 << 27))));

Please update this to use the enums that anton recently added for  
decoding the flags values.

>      unsigned CurArgOffset = ArgOffset;
>      switch (ObjectVT) {
>      default: assert(0 && "Unhandled argument type!");
>      case MVT::i32:
> +      // Double word align in ELF
> +      if (Expand && !isELF_ABI && !isPPC64) GPR_idx += (GPR_idx % 2);

This says "!isELF_ABI", shouldn't it be "isELF_ABI"?  If not, you're  
modifying the Darwin/PPC ABI.

> -
> +    unsigned Flags = cast<ConstantSDNode>(Op.getOperand(5+2*i+1))- 
> >getValue();
> +    // See if next argument requires stack alignment in ELF
> +    unsigned Expand = (Arg.getValueType() == MVT::f64) ||
> +      ((i + 1 < NumOps) &&
> +      (cast<ConstantSDNode>(Op.getOperand(5+2*(i+1)+1))->getValue()
> +                                                            & (1  
> << 27)) &&
> +      (!(Flags & (1 << 27))));

Likewise, plz use enums here.  Also, there is some funky indentation  
going on here.  Perhaps making a "ConstantSDNode *Tmp" would make  
this more natural.

>      // PtrOff will be used to store the current argument to the  
> stack if a
>      // register cannot be found for it.
> -    SDOperand PtrOff = DAG.getConstant(ArgOffset,  
> StackPtr.getValueType());
> +    SDOperand PtrOff;
> +
> +    // Stack align in ELF
> +    if (isELF_ABI && Expand && !isPPC64)
> +        PtrOff = DAG.getConstant(ArgOffset + ((ArgOffset/4) % 2) *  
> PtrByteSize,
> +            StackPtr.getValueType());
> +    else
> +        PtrOff = DAG.getConstant(ArgOffset, StackPtr.getValueType());
> +

Funky indentation.  Statements should be indented by 2.   
Subexpressions (StackPtr.getValueType() should be aligned to the (.

Otherwise, looks great, thanks!

-Chris