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

[Stepan Dyatkovskiy]

Hello Manman,

I fixed the patch. Below I describe what I did.

First of all: added comments for ByValRegs collection.

 >> +      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.

"While" loop moved into nextInRegsParam(). Also I created helper 
"skipWastedByValRegs" that allows to skip records with wasted registers 
after all HandleByVal calls but before the analysis loop.
So before loop you call skipWastedByValRegs, and then you call 
nextInRegsParam and it skips records with wasted registers.

>> +        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.

The "if (CurByValIdx + 1" I replaced with this one:
[code]
   // "offset" value would be useful if byval parameter outsides
   // the registers area. Not in that case nextInRegsParam() will not
   // skip any waste registers, since parameter fill-up the whole rest
   // of registers set.
   unsigned RegsUsed = RegEnd - RegBegin;
   if (!CCInfo.nextInRegsParam() && Flags.getByValSize() > 4*RegsUsed )
     offset = RegEnd - RegBegin;
[/code]

"if (Flags.getByValSize() - 4*offset > 0)" may be simplified then to
"if (offset)".

>> -  if ((!State->isFirstByValRegValid()) &&
>> +  if ((State->getByValRegsEnd(ARM::R0, ARM::R4) != ARM::R4) &&

I've removed getByValRegsEnd, and code above replaced with next one:

[code]
if ((ARM::R0 <= reg) && (reg <= ARM::R3)) {
   // All registers reserved for byval parameters should be allocated.
   // Check that first unallocated register is higher that last one used
   // for byval parameters.
   {
     unsigned InRegsParamsCount = State->getInRegsParamsCount();
     if (InRegsParamsCount) {
       unsigned RB, RE;
       State->getInRegsParamInfo(InRegsParamsCount-1, RB, RE);
       assert(reg >= RE);
     }
   }
   //...
[/code]

>
>> +      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.
Done.

>>  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?

I've renamed ByValArgNo to InRegsParamRecordIdx. Index that refers to 
wasted record treated as invalid by getInRegsParamsCount. I've inserted 
"assert" in the last one.

>
>> -  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.

As I understand it properly:
VarArgStyleRegisters is dual purpose method.
It emits set of instructions that saves byval registers into stack. Or, 
for VA function it should save all unallocated registers in (r0-r3] 
range to the stack.

Before my patch these two cases were differentiated with next way:

We check the result of CCInfo.isFirstByValRegValid().
If it is true: ok, we have byval parameter, so just save byval registers.
If it is false, there is no byval parameters then, so then we detected 
that this method was called from ARMTargetLowering::LowerFormalArguments 
tail. It means that function is VA. And it means that we need to save 
all unallocated registers. For me that was a little bit tricky behaviour :-)

>
>> -    // 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?
Recovered, but a little bit modified, since I've split
VarArgStyleRegisters: VarArgStyleRegisters itself and StoreByValRegs.

>
>> +// 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?
Yes. The purpose of this check is to determine: was the byval parameter 
proceed already or not. Now we have set of byval parameters, so I 
replaced it with "if (CurByValIndex < ByValCount)"

Reworked patch is attached.

Thanks!
-Stepan.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr15293-2013-04-09.patch
Type: text/x-diff
Size: 21912 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130409/017681db/attachment.patch>