Fix PR15293: ARM codegen ice - expected larger existing stack allocation
Manman Ren
mren at apple.com
Wed Apr 10 16:34:54 PDT 2013
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.
> +
> + // "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.
...
> - 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?
> -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.
> - // 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.
> - AFI->setVarArgsRegSaveSize(VARegSaveSize);
> - AFI->setVarArgsFrameIndex(MFI->CreateFixedObject(VARegSaveSize,
> - ArgOffset + VARegSaveSize
> - - VARegSize,
> - false));
> - SDValue FIN = DAG.getFrameIndex(AFI->getVarArgsFrameIndex(),
> - getPointerTy());
> + int FrameIndex = MFI->CreateFixedObject(
> + VARegSaveSize, ArgOffset + VARegSaveSize - VARegSize,
> + false);
> + SDValue FIN = DAG.getFrameIndex(FrameIndex, getPointerTy());
Keep this and the change at @@ -2661,15 +2706,39 @@ in the refactoring patch.
Could you add a testing cases in 2013-04-05-Small-ByVal-Structs-PR15293.ll, with 2 byval parameters?
Once again, thanks for working on this,
Manman
On Apr 9, 2013, at 8:42 AM, Stepan Dyatkovskiy wrote:
> 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.
>
>
>
> <pr15293-2013-04-09.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130410/90f34b23/attachment.html>
More information about the llvm-commits
mailing list