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

[Nicolas Geoffray]

Here's the final patch with the modifications you suggested. Thx a lot 
for your reviewing Chris.

If everything's OK I'm checking this in soon.

Cheers,
Nicolas

Chris Lattner wrote:
>
> 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: calling-conv-ELF.patch
Type: text/x-patch
Size: 7811 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070312/19417c7f/attachment.bin>