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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
