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

[Manman Ren]

On Apr 12, 2013, at 5:15 AM, Stepan Dyatkovskiy wrote:

> Hello Manman,
> 
> I reattached full patch with fixes.
> 
> 1. I've removedskipWastedByValRegs, addByValWastedRegs and Wasted flag.
> 2. Fixed usage of "offset" variable in LowerFormalArguments
> 3. And fixed regression test: added function with two byval structures.
The above looks good.
> 
> *4.* One new change I've made is
> CC_ARM_AAPCS_Common rule.
> 
> Currently, for align == 4 and parameter type <=i32, we try to assign it to the first unallocated register from [r0-r3] range, but only if getNextStackOffset() == 0.
> I suppose that "getNextStackOffset() != 0" is treated as fact? that all registers are used? and params goes to the stack now. After byval structures will be implemented, the next case is possible:
> 
> void @f(%i32_struct* byval, i32)
> 
> Here, we need to allocate stack for first parameter, but after we still have unallocated registers. And second parameter should be sent to r1. Current CC_ARM_AAPCS_Common implementation will send second parameter to the stack.
> Though, I'm still not sure about this change, just because may be there are more reasons to check nextStackOffset value.
Can we fix the above issue in a separate patch with additional testing case?
> 
> >>> +    // 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.
> How do you think, do we need this assertion?
I don't really think it is necessary :)
> 
> I didn't present refactor patch, just because main patch didn't get LGTM community and hypothetically may contain some mistakes..
LGTM otherwise.

Thanks,
Manman
> 
> -Stepan
> 
> Stepan Dyatkovskiy wrote:
>> 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.
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> <pr15293-2013-04-12.patch>