<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">- unsigned FirstByValReg;</font></div><div><font class="Apple-style-span" color="#000000">- bool FirstByValRegValid;</font></div><div><font class="Apple-style-span" color="#000000"><br></font></div><div><font class="Apple-style-span" color="#000000">+ struct ByValInfo {</font></div><div><font class="Apple-style-span" color="#000000">+ ByValInfo(unsigned B, unsigned E, bool IsWaste = false) :</font></div><div><font class="Apple-style-span" color="#000000">+ Begin(B), End(E), Waste(IsWaste) {}</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned Begin;</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned End;</font></div><div><font class="Apple-style-span" color="#000000">+ bool Waste;</font></div><div><font class="Apple-style-span" color="#000000">+ };</font></div><div><font class="Apple-style-span" color="#000000">+ SmallVector<ByValInfo, 4 > ByValRegs;</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned InRegsParamsProceed;</font></div><div><font class="Apple-style-span" color="#000000">+</font></div></blockquote>Could you add comments to the above fields? Please say whether End is inclusive [Begin, End) or [Begin, End].<div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ // Get information about N-th byval parameter that is stored in registers.</font></div><div><font class="Apple-style-span" color="#000000">+ // Here "ByValParamIndex" is N.</font></div><div><font class="Apple-style-span" color="#000000">+ bool getInRegsParamInfo(unsigned ByValParamIndex,</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned& BeginReg, unsigned& EndReg) {</font></div></blockquote>Can you make this const? Please add comments about the return value of this function. Looks like the return value </div><div>is used to set WatedRegs, can we return Waste instead of !Waste?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ while (WastedRegs && CurByValIdx < ByValArgsCount) {</font></div><div><font class="Apple-style-span" color="#000000">+ WastedRegs = !CCInfo.getInRegsParamInfo(CurByValIdx, RegBegin, RegEnd);</font></div><div><font class="Apple-style-span" color="#000000">+ assert(RegBegin >= ARM::R0 && RegBegin < RegEnd && RegEnd <= ARM::R4</font></div><div><font class="Apple-style-span" color="#000000">+ && "ByVal registers may lie in range [R0, R3) only");</font></div><div><font class="Apple-style-span" color="#000000">+ if (WastedRegs) {</font></div><div><font class="Apple-style-span" color="#000000">+ ++CurByValIdx;</font></div><div><font class="Apple-style-span" color="#000000">+ CCInfo.nextInRegsParam();</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div></blockquote>Could you add comments here? Can we make this into a function? This is used later on.</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ if (CurByValIdx + 1 == ByValArgsCount)</font></div><div><font class="Apple-style-span" color="#000000">+ offset = RegEnd - RegBegin;</font></div></blockquote><blockquote type="cite"><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 (Flags.getByValSize() > 4*offset) {</font></div></blockquote>This does not look correct. The default value of offset is 0, should we set offset when it is not the last byval parameter?</div><div>I assume we do not need to enter the if statement for byval parameters that are all in registers.</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">+ if ((State->getByValRegsEnd(ARM::R0, ARM::R4) != ARM::R4) &&</font></div></blockquote>The function definition of getByValRegsEnd is not really straight-forward. Could you add some comments here?</div><div>It is effectively checking if there exists any register that can be used for this byval parameter.</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ unsigned ByValRegBegin = reg;</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned ByValRegEnd = (size < excess) ? reg + size/4 : ARM::R4;</font></div></blockquote>Comments about ByValRegEnd. If we do not have enough registers, it is clamped at ARM::R4.</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::computeRegArea(CCState &CCInfo, MachineFunction &MF,</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned ByValArgNo,</font></div><div><font class="Apple-style-span" color="#000000"> unsigned &VARegSize, unsigned &VARegSaveSize)</font></div><div><font class="Apple-style-span" color="#000000"> const {</font></div><div><font class="Apple-style-span" color="#000000"> unsigned NumGPRs;</font></div><div><font class="Apple-style-span" color="#000000">- if (CCInfo.isFirstByValRegValid())</font></div><div><font class="Apple-style-span" color="#000000">- NumGPRs = ARM::R4 - CCInfo.getFirstByValReg();</font></div><div><font class="Apple-style-span" color="#000000">+ if (ByValArgNo < CCInfo.getInRegsParamsCount()) {</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned RBegin, REnd;</font></div><div><font class="Apple-style-span" color="#000000">+ CCInfo.getInRegsParamInfo(ByValArgNo, RBegin, REnd);</font></div><div><font class="Apple-style-span" color="#000000">+ NumGPRs = REnd - RBegin;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div></blockquote>Does ByValArgNo include the waste area added in addByValWastedRegs?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">- unsigned firstRegToSaveIndex;</font></div><div><font class="Apple-style-span" color="#000000">- if (CCInfo.isFirstByValRegValid())</font></div><div><font class="Apple-style-span" color="#000000">- firstRegToSaveIndex = CCInfo.getFirstByValReg() - ARM::R0;</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned firstRegToSaveIndex, lastRegToSaveIndex;</font></div><div><font class="Apple-style-span" color="#000000">+ unsigned RBegin, REnd;</font></div><div><font class="Apple-style-span" color="#000000">+ if (ByValArgNo < CCInfo.getInRegsParamsCount()) {</font></div><div><font class="Apple-style-span" color="#000000">+ CCInfo.getInRegsParamInfo(ByValArgNo, RBegin, REnd);</font></div><div><font class="Apple-style-span" color="#000000">+ firstRegToSaveIndex = RBegin - ARM::R0;</font></div><div><font class="Apple-style-span" color="#000000">+ lastRegToSaveIndex = REnd - ARM::R0;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div></blockquote></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000"> else {</font></div><div><font class="Apple-style-span" color="#000000"> firstRegToSaveIndex = CCInfo.getFirstUnallocated</font></div><div><font class="Apple-style-span" color="#000000"> (GPRArgRegs, sizeof(GPRArgRegs) / sizeof(GPRArgRegs[0]));</font></div><div><font class="Apple-style-span" color="#000000">+ lastRegToSaveIndex = 4;</font></div><div><font class="Apple-style-span" color="#000000"> }</font></div></blockquote></div><div><div>Do you know what this else clause is for? I don't quite understand the update to firstRegToSaveIndex here.</div></div><div><br></div><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></blockquote>Why remove the comments?</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+// Setup stack frame, the va_list pointer will start from.</font></div><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">+ 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">+ MachineFunction &MF = DAG.getMachineFunction();</font></div><div><font class="Apple-style-span" color="#000000">+ ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ int FrameIndex =</font></div><div><font class="Apple-style-span" color="#000000">+ StoreByValRegs(CCInfo, DAG, dl, Chain, 0, CCInfo.getInRegsParamsCount(),</font></div><div><font class="Apple-style-span" color="#000000">+ 0, ArgOffset, ForceMutable);</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ AFI->setVarArgsFrameIndex(FrameIndex);</font></div><div><font class="Apple-style-span" color="#000000">+}</font></div><div><font class="Apple-style-span" color="#000000">+</font></div></blockquote>Can we do this factoring in a pre-patch?</div><div><br></div><div><div><blockquote type="cite"><font class="Apple-style-span" color="#000000">- if (!AFI->getVarArgsFrameIndex()) {</font></blockquote>Is it okay to remove this if statement?</div></div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ while (WastedRegs && CurByValIndex < ByValCount) {</font></div><div><font class="Apple-style-span" color="#000000">+ WastedRegs =</font></div><div><font class="Apple-style-span" color="#000000">+ !CCInfo.getInRegsParamInfo(CurByValIndex, RegBegin, RegEnd);</font></div><div><font class="Apple-style-span" color="#000000">+ assert(RegBegin >= ARM::R0 &&</font></div><div><font class="Apple-style-span" color="#000000">+ RegBegin < RegEnd &&</font></div><div><font class="Apple-style-span" color="#000000">+ RegEnd <= ARM::R4 &&</font></div><div><font class="Apple-style-span" color="#000000">+ "ByVal registers may lie in range [R0, R3) only");</font></div><div><font class="Apple-style-span" color="#000000">+ if (WastedRegs) {</font></div><div><font class="Apple-style-span" color="#000000">+ ++CurByValIndex;</font></div><div><font class="Apple-style-span" color="#000000">+ CCInfo.nextInRegsParam();</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div></blockquote>Please use a helper function.</div><div><br></div><div>Thanks for working on this,</div><div>Manman</div><div><div><div><div><br></div></div><div><div>On Apr 5, 2013, at 5:24 AM, Stepan Dyatkovskiy wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi all!<br>Please find new patch in attachment.<br>Patch adds support for multiple small byval parameters that are enough small to be stored in register. It replaces the term of "FirstByValRegister" and friends with collection of ByValInfo structure instances.<br>ByValInfo contains information about single byval parameter that is stored in registers.<br>Patch also introduces the difference between actions: allocate-stack-frame-for-byval-param and allocate-stack-frame for var-arg registers.<br>In this patch you can also see how different use-cases of<br>ARMFunctionInfo::VarArgsRegSaveSize and ARMFunctionInfo::VarArgsFrameIndex.<br>I propose to rename the first one to "ByValRegSaveSize", since main role of this property is to generate prologue and epilogue properly, Since, while prologue or epilogue are emitted this field answers on question "how much registers are used for byval parameters?".<br><br>Patch also fixes PR15293, so regression test is included.<br><br>-Stepan.<br></div></blockquote></div></div></div></body></html>