Fix PR15293: ARM codegen ice - expected larger existing stack allocation

[Manman Ren]

> -  unsigned FirstByValReg;
> -  bool FirstByValRegValid;
> 
> +  struct ByValInfo {
> +    ByValInfo(unsigned B, unsigned E, bool IsWaste = false) :
> +      Begin(B), End(E), Waste(IsWaste) {}
> +    unsigned Begin;
> +    unsigned End;
> +    bool Waste;
> +  };
> +  SmallVector<ByValInfo, 4 > ByValRegs;
> +  unsigned InRegsParamsProceed;
> +
Could you add comments to the above fields? Please say whether End is inclusive [Begin, End) or [Begin, End].

> +  // Get information about N-th byval parameter that is stored in registers.
> +  // Here "ByValParamIndex" is N.
> +  bool getInRegsParamInfo(unsigned ByValParamIndex,
> +                          unsigned& BeginReg, unsigned& EndReg) {
Can you make this const? Please add comments about the return value of this function. Looks like the return value 
is used to set WatedRegs, can we return Waste instead of !Waste?

> +      while (WastedRegs && CurByValIdx < ByValArgsCount) {
> +        WastedRegs = !CCInfo.getInRegsParamInfo(CurByValIdx, RegBegin, RegEnd);
> +        assert(RegBegin >= ARM::R0 && RegBegin < RegEnd && RegEnd <= ARM::R4
> +               && "ByVal registers may lie in range [R0, R3) only");
> +        if (WastedRegs) {
> +          ++CurByValIdx;
> +          CCInfo.nextInRegsParam();
> +        }
> +      }
Could you add comments here? Can we make this into a function? This is used later on.

> +        if (CurByValIdx + 1 == ByValArgsCount)
> +          offset = RegEnd - RegBegin;
>        }
> 
> -      if (Flags.getByValSize() - 4*offset > 0) {
> +      if (Flags.getByValSize() > 4*offset) {
This does not look correct. The default value of offset is 0, should we set offset when it is not the last byval parameter?
I assume we do not need to enter the if statement for byval parameters that are all in registers.

> -  if ((!State->isFirstByValRegValid()) &&
> +  if ((State->getByValRegsEnd(ARM::R0, ARM::R4) != ARM::R4) &&
The function definition of getByValRegsEnd is not really straight-forward. Could you add some comments here?
It is effectively checking if there exists any register that can be used for this byval parameter.

> +      unsigned ByValRegBegin = reg;
> +      unsigned ByValRegEnd = (size < excess) ? reg + size/4 : ARM::R4;
Comments about ByValRegEnd. If we do not have enough registers, it is clamped at ARM::R4.

>  void
>  ARMTargetLowering::computeRegArea(CCState &CCInfo, MachineFunction &MF,
> +                                  unsigned ByValArgNo,
>                                    unsigned &VARegSize, unsigned &VARegSaveSize)
>    const {
>    unsigned NumGPRs;
> -  if (CCInfo.isFirstByValRegValid())
> -    NumGPRs = ARM::R4 - CCInfo.getFirstByValReg();
> +  if (ByValArgNo < CCInfo.getInRegsParamsCount()) {
> +    unsigned RBegin, REnd;
> +    CCInfo.getInRegsParamInfo(ByValArgNo, RBegin, REnd);
> +    NumGPRs = REnd - RBegin;
> +  }
Does ByValArgNo include the waste area added in addByValWastedRegs?

> -  unsigned firstRegToSaveIndex;
> -  if (CCInfo.isFirstByValRegValid())
> -    firstRegToSaveIndex = CCInfo.getFirstByValReg() - ARM::R0;
> +  unsigned firstRegToSaveIndex, lastRegToSaveIndex;
> +  unsigned RBegin, REnd;
> +  if (ByValArgNo < CCInfo.getInRegsParamsCount()) {
> +    CCInfo.getInRegsParamInfo(ByValArgNo, RBegin, REnd);
> +    firstRegToSaveIndex = RBegin - ARM::R0;
> +    lastRegToSaveIndex = REnd - ARM::R0;
> +  }

>    else {
>      firstRegToSaveIndex = CCInfo.getFirstUnallocated
>        (GPRArgRegs, sizeof(GPRArgRegs) / sizeof(GPRArgRegs[0]));
> +    lastRegToSaveIndex = 4;
>    }

Do you know what this else clause is for? I don't quite understand the update to firstRegToSaveIndex here.

> -    // If this function is vararg, store any remaining integer argument regs
> -    // to their spots on the stack so that they may be loaded by deferencing
> -    // the result of va_next.
Why remove the comments?

> +// Setup stack frame, the va_list pointer will start from.
> +void
> +ARMTargetLowering::VarArgStyleRegisters(CCState &CCInfo, SelectionDAG &DAG,
> +                                        DebugLoc dl, SDValue &Chain,
> +                                        unsigned ArgOffset,
> +                                        bool ForceMutable) const {
> +  MachineFunction &MF = DAG.getMachineFunction();
> +  ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
> +
> +  int FrameIndex =
> +    StoreByValRegs(CCInfo, DAG, dl, Chain, 0, CCInfo.getInRegsParamsCount(),
> +                   0, ArgOffset, ForceMutable);
> +
> +  AFI->setVarArgsFrameIndex(FrameIndex);
> +}
> +
Can we do this factoring in a pre-patch?

> -            if (!AFI->getVarArgsFrameIndex()) {
Is it okay to remove this if statement?

> +            while (WastedRegs && CurByValIndex < ByValCount) {
> +              WastedRegs =
> +              !CCInfo.getInRegsParamInfo(CurByValIndex, RegBegin, RegEnd);
> +              assert(RegBegin >= ARM::R0 &&
> +                     RegBegin < RegEnd &&
> +                     RegEnd <= ARM::R4 &&
> +                     "ByVal registers may lie in range [R0, R3) only");
> +              if (WastedRegs) {
> +                ++CurByValIndex;
> +                CCInfo.nextInRegsParam();
> +              }
> +            }
Please use a helper function.

Thanks for working on this,
Manman

On Apr 5, 2013, at 5:24 AM, Stepan Dyatkovskiy wrote:

> Hi all!
> Please find new patch in attachment.
> Patch adds support for multiple small byval parameters that are enough small to be stored in register. It replaces the term of "FirstByValRegister" and friends with collection of ByValInfo structure instances.
> ByValInfo contains information about single byval parameter that is stored in registers.
> Patch also introduces the difference between actions: allocate-stack-frame-for-byval-param and allocate-stack-frame for var-arg registers.
> In this patch you can also see how different use-cases of
> ARMFunctionInfo::VarArgsRegSaveSize and ARMFunctionInfo::VarArgsFrameIndex.
> I propose to rename the first one to "ByValRegSaveSize", since main role of this property is to generate prologue and epilogue properly, Since, while prologue or epilogue are emitted this field answers on question "how much registers are used for byval parameters?".
> 
> Patch also fixes PR15293, so regression test is included.
> 
> -Stepan.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130408/e6cbd535/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr15293-2013-04-05-3.patch
Type: text/x-diff
Size: 18780 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130408/e6cbd535/attachment.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130408/e6cbd535/attachment-0001.html>