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

[Stepan Dyatkovskiy]

Manman Ren wrote:
>
> Hi Stepan,
>
> One high-level question: do we need to insert the wasted record to
> ByValRegs and later on skip those records?
> It seems to me that we can only insert the records for the actual byval
> parameters.
> If that is true, we can remove addByValWastedRegs and skipWastedByValRegs.
Hm.. yes its reasonable. I'll try to fix.

>
>> +
>> +        // "offset" value would be usefull if byval parameter outsides
>> +        // the registers area. Note, in that case nextInRegsParam()
>> will not
>> +        // skip any waste registers, since parameter fillup the whole
>> rest
>> +        // of registers set.
>> +        unsigned RegsUsed = RegEnd - RegBegin;
>> +        if (!CCInfo.nextInRegsParam() && Flags.getByValSize() >
>> 4*RegsUsed )
>> +          offset = RegEnd - RegBegin;
>>        }
>>
>> -      if (Flags.getByValSize() - 4*offset > 0) {
>> +      if (offset) {
>
> This still does not look right.
> There are 3 cases:
> If the byval parameter is in registers partially, we should create a
> COPY_STRUCT_BYVAL with the correct offset;
> If the byval parameter is all in registers, there is no need to create a
> COPY_STRUCT_BYVAL;
> If the byval parameter is all on stack, we should create a
> COPY_STRUCT_BYVAL with offset equal to 0.
>
> When looking at the above code, offset is default to zero, if
> CurByValIdx >= ByValArgsCount (this byval parameter is all on stack),
> offset will still be zero,
> and we will not create a COPY_STRUCT_BYVAL.
> The correct logic seems to be always updating offset inside the if and
> check Flags.getByValSize() > 4*offset
> if (CurByValIdx < ByValArgsCount) {
>    …
>    offset = RegEnd - RegBegin; //number of registers occupied by this
> byval parameter
>    CCInfo.nextInRegsParam(); // go to the next byval parameter that is
> in registers partially or all
> }
> if (Flags.getByValSize() > 4*offset) {
>    // Part of the byval parameter is on stack.
>    ...
Oh.. I missed third case. Then we keep first variant here.

>> -  if ((!State->isFirstByValRegValid()) &&
>> -      (ARM::R0 <= reg) && (reg <= ARM::R3)) {
>> +  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);
>> +      }
>> +    }
> Is this only for assertion? Is this necessary?
Yes. I've just moved this check from "if" statement, since it must be 
true always.

>> -void
>> -ARMTargetLowering::VarArgStyleRegisters(CCState &CCInfo, SelectionDAG
>> &DAG,
>> -                                        DebugLoc dl, SDValue &Chain,
>> -                                        const Value *OrigArg,
>> -                                        unsigned OffsetFromOrigArg,
>> -                                        unsigned ArgOffset,
>> -                                        bool ForceMutable) const {
>> +// Return: The frame index registers were stored into.
>> +int
>> +ARMTargetLowering::StoreByValRegs(CCState &CCInfo, SelectionDAG &DAG,
>> +                                  DebugLoc dl, SDValue &Chain,
>> +                                  const Value *OrigArg,
>> +                                  unsigned InRegsParamRecordIdx,
>> +                                  unsigned OffsetFromOrigArg,
>> +                                  unsigned ArgOffset,
>> +                                  bool ForceMutable) const {
>
> It would be much cleaner if we can move the refactoring of
> VarArgStyleRegisters to a separate patch.
For sure.

P.S.: Today I had some troubles with electricity, so I'll try present 
new patch tomorrow.

-Stepan.