<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div>Hi Stepan,</div><div><br></div><div>One high-level question: do we need to insert the wasted record to ByValRegs and later on skip those records?</div><div>It seems to me that we can only insert the records for the actual byval parameters.</div><div>If that is true, we can remove addByValWastedRegs and skipWastedByValRegs.</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ // "offset" value would be usefull if byval parameter outsides</font></div><div><font class="Apple-style-span" color="#000000">+ // the registers area. Note, in that case nextInRegsParam() will not</font></div><div><font class="Apple-style-span" color="#000000">+ // skip any waste registers, since parameter fillup the whole rest</font></div><div><font class="Apple-style-span" color="#000000">+ // of registers set.</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned RegsUsed = RegEnd - RegBegin;</font></div><div><font class="Apple-style-span" color="#000000">+ if (!CCInfo.nextInRegsParam() && Flags.getByValSize() > 4*RegsUsed )</font></div><div><font class="Apple-style-span" color="#000000">+ offset = RegEnd - RegBegin;</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div><div><font class="Apple-style-span" color="#000000"><br></font></div><div><font class="Apple-style-span" color="#000000">- if (Flags.getByValSize() - 4*offset > 0) {</font></div><div><font class="Apple-style-span" color="#000000">+ if (offset) {</font></div></blockquote><br>This still does not look right.</div><div>There are 3 cases:</div><div>If the byval parameter is in registers partially, we should create a COPY_STRUCT_BYVAL with the correct offset;</div><div>If the byval parameter is all in registers, there is no need to create a COPY_STRUCT_BYVAL;</div><div>If the byval parameter is all on stack, we should create a COPY_STRUCT_BYVAL with offset equal to 0.</div><div><br></div><div>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,</div><div>and we will not create a COPY_STRUCT_BYVAL.</div><div>The correct logic seems to be always updating offset inside the if and check Flags.getByValSize() > 4*offset</div><div>if (CurByValIdx < ByValArgsCount) {</div><div> …</div><div> offset = RegEnd - RegBegin; //number of registers occupied by this byval parameter</div><div> CCInfo.nextInRegsParam(); // go to the next byval parameter that is in registers partially or all</div><div>}</div><div>if (Flags.getByValSize() > 4*offset) {</div><div> // Part of the byval parameter is on stack.</div><div> ...</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">- if ((!State->isFirstByValRegValid()) &&</font></div><div><font class="Apple-style-span" color="#000000">- (ARM::R0 <= reg) && (reg <= ARM::R3)) {</font></div><div><font class="Apple-style-span" color="#000000">+ if ((ARM::R0 <= reg) && (reg <= ARM::R3)) {</font></div><div><font class="Apple-style-span" color="#000000">+ // All registers reserved for byval parameters should be allocated.</font></div><div><font class="Apple-style-span" color="#000000">+ // Check that first unallocated register is higher that last one used</font></div><div><font class="Apple-style-span" color="#000000">+ // for byval parameters.</font></div><div><font class="Apple-style-span" color="#000000">+ {</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned InRegsParamsCount = State->getInRegsParamsCount();</font></div><div><font class="Apple-style-span" color="#000000">+ if (InRegsParamsCount) {</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned RB, RE;</font></div><div><font class="Apple-style-span" color="#000000">+ State->getInRegsParamInfo(InRegsParamsCount-1, RB, RE);</font></div><div><font class="Apple-style-span" color="#000000">+ assert(reg >= RE);</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div></blockquote>Is this only for assertion? Is this necessary?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">-void</font></div><div><font class="Apple-style-span" color="#000000">-ARMTargetLowering::VarArgStyleRegisters(CCState &CCInfo, SelectionDAG &DAG,</font></div><div><font class="Apple-style-span" color="#000000">- DebugLoc dl, SDValue &Chain,</font></div><div><font class="Apple-style-span" color="#000000">- const Value *OrigArg,</font></div><div><font class="Apple-style-span" color="#000000">- unsigned OffsetFromOrigArg,</font></div><div><font class="Apple-style-span" color="#000000">- unsigned ArgOffset,</font></div><div><font class="Apple-style-span" color="#000000">- bool ForceMutable) const {</font></div><div><font class="Apple-style-span" color="#000000">+// Return: The frame index registers were stored into.</font></div><div><font class="Apple-style-span" color="#000000">+int</font></div><div><font class="Apple-style-span" color="#000000">+ARMTargetLowering::StoreByValRegs(CCState &CCInfo, SelectionDAG &DAG,</font></div><div><font class="Apple-style-span" color="#000000">+ DebugLoc dl, SDValue &Chain,</font></div><div><font class="Apple-style-span" color="#000000">+ const Value *OrigArg,</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned InRegsParamRecordIdx,</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned OffsetFromOrigArg,</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned ArgOffset,</font></div><div><font class="Apple-style-span" color="#000000">+ bool ForceMutable) const {</font></div></blockquote><div><br></div><div>It would be much cleaner if we can move the refactoring of VarArgStyleRegisters to a separate patch.</div><div><br></div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">- // If this function is vararg, store any remaining integer argument regs</font></div><div><font class="Apple-style-span" color="#000000">- // to their spots on the stack so that they may be loaded by deferencing</font></div><div><font class="Apple-style-span" color="#000000">- // the result of va_next.</font></div><div><font class="Apple-style-span" color="#000000">- AFI->setVarArgsRegSaveSize(VARegSaveSize);</font></div><div><font class="Apple-style-span" color="#000000">- AFI->setVarArgsFrameIndex(MFI->CreateFixedObject(VARegSaveSize,</font></div><div><font class="Apple-style-span" color="#000000">- ArgOffset + VARegSaveSize</font></div><div><font class="Apple-style-span" color="#000000">- - VARegSize,</font></div><div><font class="Apple-style-span" color="#000000">- false));</font></div><div><font class="Apple-style-span" color="#000000">- SDValue FIN = DAG.getFrameIndex(AFI->getVarArgsFrameIndex(),</font></div><div><font class="Apple-style-span" color="#000000">- getPointerTy());</font></div><div><font class="Apple-style-span" color="#000000">+ int FrameIndex = MFI->CreateFixedObject(</font></div><div><font class="Apple-style-span" color="#000000">+ VARegSaveSize, ArgOffset + VARegSaveSize - VARegSize,</font></div><div><font class="Apple-style-span" color="#000000">+ false);</font></div><div><font class="Apple-style-span" color="#000000">+ SDValue FIN = DAG.getFrameIndex(FrameIndex, getPointerTy());</font></div></blockquote></div><div>Keep this and the change at @@ -2661,15 +2706,39 @@ in the refactoring patch.</div><div><br></div><div>Could you add a testing cases in 2013-04-05-Small-ByVal-Structs-PR15293.ll, with 2 byval parameters?</div><div><br></div><div>Once again, thanks for working on this,</div><div>Manman</div><div><br><div><div>On Apr 9, 2013, at 8:42 AM, Stepan Dyatkovskiy wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hello Manman,<br><br>I fixed the patch. Below I describe what I did.<br><br>First of all: added comments for ByValRegs collection.<br><br>>> + while (WastedRegs && CurByValIdx < ByValArgsCount) {<br><blockquote type="cite"><blockquote type="cite">+ WastedRegs = !CCInfo.getInRegsParamInfo(CurByValIdx,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">RegBegin, RegEnd);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ assert(RegBegin >= ARM::R0 && RegBegin < RegEnd && RegEnd <=<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">ARM::R4<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ && "ByVal registers may lie in range [R0, R3) only");<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (WastedRegs) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ++CurByValIdx;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ CCInfo.nextInRegsParam();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite">Could you add comments here? Can we make this into a function? This is<br></blockquote><blockquote type="cite">used later on.<br></blockquote><br>"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.<br>So before loop you call skipWastedByValRegs, and then you call nextInRegsParam and it skips records with wasted registers.<br><br><blockquote type="cite"><blockquote type="cite">+ if (CurByValIdx + 1 == ByValArgsCount)<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ offset = RegEnd - RegBegin;<br></blockquote></blockquote>>> ...<br><blockquote type="cite"><blockquote type="cite"> }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- if (Flags.getByValSize() - 4*offset > 0) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (Flags.getByValSize() > 4*offset) {<br></blockquote></blockquote><blockquote type="cite">This does not look correct. The default value of offset is 0, should we<br></blockquote><blockquote type="cite">set offset when it is not the last byval parameter?<br></blockquote><blockquote type="cite">I assume we do not need to enter the if statement for byval parameters<br></blockquote><blockquote type="cite">that are all in registers.<br></blockquote><br>The "if (CurByValIdx + 1" I replaced with this one:<br>[code]<br> // "offset" value would be useful if byval parameter outsides<br> // the registers area. Not in that case nextInRegsParam() will not<br> // skip any waste registers, since parameter fill-up the whole rest<br> // of registers set.<br> unsigned RegsUsed = RegEnd - RegBegin;<br> if (!CCInfo.nextInRegsParam() && Flags.getByValSize() > 4*RegsUsed )<br> offset = RegEnd - RegBegin;<br>[/code]<br><br>"if (Flags.getByValSize() - 4*offset > 0)" may be simplified then to<br>"if (offset)".<br><br><blockquote type="cite"><blockquote type="cite">- if ((!State->isFirstByValRegValid()) &&<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if ((State->getByValRegsEnd(ARM::R0, ARM::R4) != ARM::R4) &&<br></blockquote></blockquote><br>I've removed getByValRegsEnd, and code above replaced with next one:<br><br>[code]<br>if ((ARM::R0 <= reg) && (reg <= ARM::R3)) {<br> // All registers reserved for byval parameters should be allocated.<br> // Check that first unallocated register is higher that last one used<br> // for byval parameters.<br> {<br> unsigned InRegsParamsCount = State->getInRegsParamsCount();<br> if (InRegsParamsCount) {<br> unsigned RB, RE;<br> State->getInRegsParamInfo(InRegsParamsCount-1, RB, RE);<br> assert(reg >= RE);<br> }<br> }<br> //...<br>[/code]<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned ByValRegBegin = reg;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned ByValRegEnd = (size < excess) ? reg + size/4 : ARM::R4;<br></blockquote></blockquote><blockquote type="cite">Comments about ByValRegEnd. If we do not have enough registers, it is<br></blockquote><blockquote type="cite">clamped at ARM::R4.<br></blockquote>Done.<br><br><blockquote type="cite"><blockquote type="cite"> void<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> ARMTargetLowering::computeRegArea(CCState &CCInfo, MachineFunction &MF,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned ByValArgNo,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> unsigned &VARegSize, unsigned<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">&VARegSaveSize)<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> const {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> unsigned NumGPRs;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- if (CCInfo.isFirstByValRegValid())<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- NumGPRs = ARM::R4 - CCInfo.getFirstByValReg();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (ByValArgNo < CCInfo.getInRegsParamsCount()) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned RBegin, REnd;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ CCInfo.getInRegsParamInfo(ByValArgNo, RBegin, REnd);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ NumGPRs = REnd - RBegin;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite">Does ByValArgNo include the waste area added in addByValWastedRegs?<br></blockquote><br>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.<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">- unsigned firstRegToSaveIndex;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- if (CCInfo.isFirstByValRegValid())<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- firstRegToSaveIndex = CCInfo.getFirstByValReg() - ARM::R0;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned firstRegToSaveIndex, lastRegToSaveIndex;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned RBegin, REnd;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ if (ByValArgNo < CCInfo.getInRegsParamsCount()) {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ CCInfo.getInRegsParamInfo(ByValArgNo, RBegin, REnd);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ firstRegToSaveIndex = RBegin - ARM::R0;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ lastRegToSaveIndex = REnd - ARM::R0;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ }<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> else {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> firstRegToSaveIndex = CCInfo.getFirstUnallocated<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> (GPRArgRegs, sizeof(GPRArgRegs) / sizeof(GPRArgRegs[0]));<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ lastRegToSaveIndex = 4;<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"> }<br></blockquote></blockquote><blockquote type="cite">Do you know what this else clause is for? I don't quite understand the<br></blockquote><blockquote type="cite">update to firstRegToSaveIndex here.<br></blockquote><br>As I understand it properly:<br>VarArgStyleRegisters is dual purpose method.<br>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.<br><br>Before my patch these two cases were differentiated with next way:<br><br>We check the result of CCInfo.isFirstByValRegValid().<br>If it is true: ok, we have byval parameter, so just save byval registers.<br>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 :-)<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">- // If this function is vararg, store any remaining integer<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">argument regs<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- // to their spots on the stack so that they may be loaded by<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">deferencing<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">- // the result of va_next.<br></blockquote></blockquote><blockquote type="cite">Why remove the comments?<br></blockquote>Recovered, but a little bit modified, since I've split<br>VarArgStyleRegisters: VarArgStyleRegisters itself and StoreByValRegs.<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">+// Setup stack frame, the va_list pointer will start from.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+void<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ARMTargetLowering::VarArgStyleRegisters(CCState &CCInfo, SelectionDAG<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">&DAG,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ DebugLoc dl, SDValue &Chain,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ unsigned ArgOffset,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ bool ForceMutable) const {<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ MachineFunction &MF = DAG.getMachineFunction();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ int FrameIndex =<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ StoreByValRegs(CCInfo, DAG, dl, Chain, 0,<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">CCInfo.getInRegsParamsCount(),<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ 0, ArgOffset, ForceMutable);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+ AFI->setVarArgsFrameIndex(FrameIndex);<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+}<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">+<br></blockquote></blockquote><blockquote type="cite">Can we do this factoring in a pre-patch?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">- if (!AFI->getVarArgsFrameIndex()) {<br></blockquote></blockquote><blockquote type="cite">Is it okay to remove this if statement?<br></blockquote>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)"<br><br>Reworked patch is attached.<br><br>Thanks!<br>-Stepan.<br><br><br><br><span><pr15293-2013-04-09.patch></span></div></blockquote></div><br></div></body></html>