<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div><br></div><div><div>On May 2, 2013, at 11:08 AM, Stepan Dyatkovskiy <<a href="mailto:stpworld@narod.ru">stpworld@narod.ru</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi Manman,<br><br><blockquote type="cite">+ // NSAA should be evaluted (NSAA means "next stacked argument address").<br>typo "evaluated"<br><br>+ // So: NextStackOffset = NSAAOffset + SizeOfByValParamsStoredInRegs.<br>+ // Then: NSAAOffset = NextStackOffset - SizeOfByValParamsStoredInRegs.<br>+ unsigned NSAAOffset = State->getNextStackOffset();<br>+ if (State->getCallOrPrologue() != Call) {<br>+ for (unsigned i = 0, e = State->getInRegsParamsCount(); i != e; ++i) {<br>+ unsigned RB, RE;<br>+ State->getInRegsParamInfo(i, RB, RE);<br>+ assert(NSAAOffset >= (RE-RB)*4 &&<br>+ "Stack offset for byval regs doesn't introduced anymore?");<br>+ NSAAOffset -= (RE-RB)*4;<br>+ }<br>+ }<br>+<br>+ if ((!Subtarget->isAAPCS_ABI() || NSAAOffset == 0) &&<br>I don't quite get the above section to update NSAAOffset. Could you give an example?<br></blockquote><br>Current implementation (even before all my patches) allocates stack area for byval registers.<br>Then, in prologue all byval parameters are stored on stack (currently single byval parameter) and used as params passed by pointer. I'm not author of this approach. There are both pros and cons. Main "pros": this behaviour relaxes registers pressure.<br></div></blockquote>Thanks for the detailed explanation.<br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>That kind of stack usage outsides AAPCS rules. AAPCS assumes that if parameter assigned to the registers only it sidesteps stack usage at all.<br><br>Example:<br><br>typedef struct { int field0; } struct1_t;<br>typedef struct { int fields[5]; } struct2_t;<br><br>foo (int a, struct_t b, int c, int d)<br><br>Reason I've added these changes is that actually NSAA from AAPCS rules is *not equal* to LLVM's CCState::StackOffset+SP:<br>int a --> r0 NSAA is SP+0, StackOffset is 0<br>struct_t b --> r1, r2 NSAA is SP+0, StackOffset := 4 (!)<br>int c --> r3 NSAA is SP+0, StackOffset is 4<br>int d --> SP, NSAA := SP+4, StackOffset := 8<br><br>Let NSAAOffset = NSAA-SP, then:<br>StackOffset = NSAAOffset + sizeof(all-by-val-regs)<br><br>The check:<br>"if ((!Subtarget->isAAPCS_ABI() || NSAAOffset == 0)"<br>is just improvement of what was before:<br>"(!Subtarget->isAAPCS_ABI() || State->getNextStackOffset() == 0)"<br><br><blockquote type="cite">Should we use up all GPRArgRegs when NSAAOffset != 0 so arguments after the byval will not be allocated to registers?<br></blockquote>If I've understood question properly:<br>If NSAA != SP and parameter size greater than all remained GPRs we send it to stack and set NCRN to R4 (so we can't use GPRs anymore).<br></div></blockquote><div><br></div>Yes, I meant the above case: NSAA != SP and byval can't all fit in GPRs.</div><div>Thanks for fixing this in attached patch.</div><div>The updated patch looks good to me.</div><div><br></div><div>Thanks,</div><div>Manman</div><div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">If NSAA != SP, but parameter could be saved in remained GPRs we follow C.4 rule and save this parameter in GPRs, increasing NCRN.<br><br>I attached examples as separated .ll files.<br><br>P.S.: While writing this post I've found that I did the mistake in HandleByVal, I changed it a bit more. Would you please look at it again. So've I reattached the patch too.<br><br>-Stepan<br><br><blockquote type="cite"><br>Otherwise LGTM.<br><br>Thanks for working on this,<br>Manman<br><br>On May 1, 2013, at 11:36 AM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Hello,<br>Please find patch in attachment that fixes byval support in ARM backend for llvm level.<br><br>-Stepan.<br><br>Stepan Dyatkovskiy wrote:<br><blockquote type="cite">Commited in r180774. Now I'm working on milestone patch, that fixes<br>byval support for llvm level. I published it before, now I need to<br>update it. Hope it come soon.<br><br>-Stepan.<br><br>Manman Ren wrote:<br><blockquote type="cite"><br>LGTM,<br><br>Thanks,<br>Manman<br><br>On Apr 28, 2013, at 10:50 AM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Hi Manman,<br>Just for keeping things actual: I reattached patch with new names.<br>-Stepan<br><br>Stepan Dyatkovskiy wrote:<br><blockquote type="cite">Hi Manman,<br><br><blockquote type="cite">Since it represents the stack area needed to save parameters that are<br></blockquote>actually passed in registers, how about RegsSaveSizeForParams or just<br>RegsSaveSize?<br><br>In AAPCS R0-R3 are called like:<br>"Argument / result / scratch register".<br>So maybe we'll use "set/getArgRegsSaveSize" names?<br><br><blockquote type="cite">Also it is weird to have "unsigned VARegSaveSize =<br></blockquote>AFI->getByValRegsSaveSize();"<br><br>Yea :-) Here replace "VAReg" with "ArgRegs": ArgRegsSaveSize,<br>ArgRegsSize.<br><br>-Stepan<br><br>Manman Ren wrote:<br><blockquote type="cite"><br>Hi Stepan,<br><br>Thanks for working on this.<br><br>I put in a comment about the refactor patch sometime ago:<br><blockquote type="cite"><blockquote type="cite">I am not sure about the renaming patch though. Functions<br>get|setVarArgsRegSaveSize are used for both vararg and byval.<br>It does not sound right to replace VarARgs with ByVal.<br></blockquote><br>Actually it is used not for vararg function, as I found it main role<br>of this param in emitPrologue, it is used for proper stack<br>allocation.<br>Stack size should enough for storing all registers, used for byval<br>parameters. And yes, if it is VA function, we must also extend this<br>stack area for all unallocated R0-R3 regs storage.<br></blockquote><br>Agree. That is why I think ByVal is not a great name either.<br>Since it represents the stack area needed to save parameters that are<br>actually passed in registers, how about RegsSaveSizeForParams or just<br>RegsSaveSize?<br>Also it is weird to have "unsigned VARegSaveSize =<br>AFI->getByValRegsSaveSize();" where we assign return value of<br>getByValRegsSaveSize to a variable "VARegSaveSize".<br><br>Manman<br><br>On Apr 26, 2013, at 11:12 AM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Hi Manman and Renato,<br>Patch was tested with test-suite all test have bee passed.<br>-Stepan<br><br>Stepan Dyatkovskiy wrote:<br><blockquote type="cite">Hi all,<br><br>AAPCS, 5.5 Parameters Passing, Stage C, C4 and C5 have been fixed,<br>r180011.<br><br>So, next patch is in attachment: VarArgsStyleRegisters refactoring.<br><br>-Stepan.<br><br>Stepan Dyatkovskiy wrote:<br><blockquote type="cite">Hello, Manman.<br>That's kind of snowball!<br>Today test-suite finished with two new failures on my beagle.<br>It helped me find out one more hidden logic dependency in ARM<br>backend.<br><br>May be its not a mistake, but look:<br><br>In ARMCallingConv.h, "f64AssignAAPCS" allocates registers for f64.<br>This<br>method is called for VA functions, since there is no CPRCs, and<br>*any*<br>parameters could be allocated in R0-R3. Either R0,R1 or R2,R3<br>could be<br>allocated here. If all the registers except R3 were allocated<br>somewhere<br>before, f64AssignAAPCS just moves whole f64 to stack, it doesn't<br>waste<br>R3, so R3 could be allocated later. But logic seems to be same<br>as in<br>ARMTargetLowering::HandleByVal.<br><br>My main fix in calling convention was:<br><br>def CC_ARM_AAPCS_Common : CallingConv<[<br>...<br>- CCIfType<[i32], CCIf<"State.getNextStackOffset() == 0 &&"<br>- "ArgFlags.getOrigAlign() != 8",<br>+ CCIfType<[i32], CCIf<"ArgFlags.getOrigAlign() != 8",<br> CCAssignToReg<[R0, R1, R2, R3]>>>,<br><br>Now I got some picture why "State.getNextStackOffset() == 0" was<br>added:<br>due to f64AssignAAPCS implementation. May be the are more reasons:<br>next<br>test-suite results will show it then :-)<br><br>Anyway, original CCIfType brakes C.4 and C.5, while my fix<br>brakes some<br>implicit statement defined in f64AssignAAPCS:<br><br>"stack-offset != 0 doesn't mean all registers are allocated"<br><br>So I've fixed f64AssignAAPCS too. Once we waste registers in<br>HandleByVal<br>(when we send parameter to stack), I think it would be good to keep<br>same<br>behaviour for f64AssignAAPCS since its HandleByVal's analogue<br>for f64<br>params.<br><br>I attached new patch, now with 3 tests :-)<br>(3rd test is 2013-04-21-AAPCS-VA-C.1.cp.ll)<br><br>-Stepan.<br><br>Manman Ren wrote:<br><blockquote type="cite"><br>LGTM,<br><br>Thanks a lot,<br>Manman<br><br>On Apr 18, 2013, at 8:41 PM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Fixed :-) New patch (+2 tests) is attached.<br>-Stepan<br><br>Manman Ren wrote:<br><blockquote type="cite"><br>"AAPCS-C.4-vs-VFP-2013-04-17.patch" looks good to me.<br><br>As a side note, we may have issues with 9 floats, 3 ints and a<br>small<br>byval.<br>The standard says we should split the byval only when NSAA is<br>equal<br>to SP (C.5), in the case of 9 floats, 3 ints and a small byval,<br>we should not split the byval since we advanced NSAA with the<br>9th<br>float.<br><br>Thanks,<br>Manman<br><br>On Apr 16, 2013, at 8:51 PM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Hi Manman,<br><br><blockquote type="cite">AAPCS-C.4-vs-VFP-2013-04-16.patch looks good except that the<br>testing case.<br>Should it have 9 doubles and 1 integer instead of a single<br>double<br>and a single integer?<br>The testing case passes even without the fix.<br></blockquote>That was my midnight mistake, sorry.<br>I reattached patch with proper test. In the end of test<br>file, I've<br>added comments for @doFoo: small description of how parameters<br>must<br>be passed:<br>first eight doubles --> D0-D7<br>9th double --> Stack<br>10th param (first i32) --> R0.<br><br>It is important to have 9 double params here, since we must<br>fill-up<br>all possible VFPs, then get compiler to use stack offset,<br>and only<br>after start use GPRs.<br><br><blockquote type="cite"><br>I am not sure about the renaming patch though. Functions<br>get|setVarArgsRegSaveSize are used for both vararg and byval.<br>It does not sound right to replace VarARgs with ByVal.<br></blockquote><br>Actually it is used not for vararg function, as I found it main<br>role of this param in emitPrologue, it is used for proper stack<br>allocation.<br>Stack size should enough for storing all registers, used for<br>byval<br>parameters. And yes, if it is VA function, we must also extend<br>this<br>stack area for all unallocated R0-R3 regs storage.<br></blockquote>Agree. That is why I think ByVal is not a great name either.<br>Since it represents the stack area needed to save parameters<br>that<br>are actually passed in registers, how about<br>RegsSaveSizeForParams or<br>just RegsSaveSize?<br>Also it is weird to have "unsigned VARegSaveSize =<br>AFI->getByValRegsSaveSize();" where we assign return value of<br>getByValRegsSaveSize to a variable "VARegSaveSize".<br><br>Thanks for working on this.<br><blockquote type="cite"><br>-Stepan.<br><br><blockquote type="cite">Thanks for working on this.<br>Manman<br><br>On Apr 15, 2013, at 9:23 AM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Hi Manman,<br><br><blockquote type="cite"><blockquote type="cite">*4.* One new change I've made is<br>CC_ARM_AAPCS_Common rule.<br></blockquote>Can we fix the above issue in a separate patch with<br>additional<br>testing case?<br></blockquote><br>Well.. We can use next case: Co-Processor register candidates<br>may be<br>either in VFP or in stack, so when all VFP are allocated<br>stack is<br>used. We can use stack without GPR allocation in that case,<br>passing 9 f64 params, for example: first eight goes to d0-d7,<br>night goes to stack. Now as 10th parameter we pass i32,<br>according<br>to AAPCS, 5.5 Parameters Passing, Stage C, C.4:<br><br>[quote]<br>If the size in words of the argument is not more than r4<br>minus<br>NCRN, the argument is copied into<br>core registers, starting at the NCRN. The NCRN is<br>incremented by<br>the number of registers used.<br>Successive registers hold the parts of the argument they<br>would<br>hold if its value were loaded into<br>those registers from memory using an LDM instruction. The<br>argument has now been allocated.<br>[/quote]<br><br>It must to R0 in our case. But currently it goes to stack.<br>I also checked this case with GCC - it works as in AAPCS<br>written.<br><br>About refactoring. I propose do it in two stages, first<br>rename<br>"getVarArgsRegSaveSize" to "get/setByValRegsSaveSize", and<br>detach<br>"StoreByValRegs" functionality from "VarArgStyleRegisters"<br>method.<br><br>I attached first refactor patch, Calling-Convention patch<br>with<br>test-case, and calling-convention .c test case.<br>Try run .c file with "arm-linux-gnueabihf-gcc" and<br>"./arm-linux-gnueabihf-clang -mcpu=cortex-a9".<br><br>P.S.: My beagleboard is very very slow, "release" build takes<br>about 15 hours, test-suite may be 15-20 hours too. I'll<br>try ask<br>somebody to present me faster device, but it take some time.<br><br>-Stepan.<br><br>Manman Ren wrote:<br><blockquote type="cite"><br>On Apr 12, 2013, at 5:15 AM, Stepan Dyatkovskiy wrote:<br><br><blockquote type="cite">Hello Manman,<br><br>I reattached full patch with fixes.<br><br>1. I've removedskipWastedByValRegs, addByValWastedRegs and<br>Wasted flag.<br>2. Fixed usage of "offset" variable in LowerFormalArguments<br>3. And fixed regression test: added function with two byval<br>structures.<br></blockquote>The above looks good.<br><blockquote type="cite"><br>*4.* One new change I've made is<br>CC_ARM_AAPCS_Common rule.<br><br>Currently, for align == 4 and parameter type <=i32, we<br>try to<br>assign it to the first unallocated register from [r0-r3]<br>range,<br>but only if getNextStackOffset() == 0.<br>I suppose that "getNextStackOffset() != 0" is treated as<br>fact?<br>that all registers are used? and params goes to the<br>stack now.<br>After byval structures will be implemented, the next<br>case is<br>possible:<br><br>void @f(%i32_struct* byval, i32)<br><br>Here, we need to allocate stack for first parameter, but<br>after<br>we still have unallocated registers. And second parameter<br>should be sent to r1. Current CC_ARM_AAPCS_Common<br>implementation will send second parameter to the stack.<br>Though, I'm still not sure about this change, just<br>because may<br>be there are more reasons to check nextStackOffset value.<br></blockquote>Can we fix the above issue in a separate patch with<br>additional<br>testing case?<br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">+ // for byval parameters.<br>+ {<br>+ unsigned InRegsParamsCount =<br>State->getInRegsParamsCount();<br>+ if (InRegsParamsCount) {<br>+ unsigned RB, RE;<br>+<br>State->getInRegsParamInfo(InRegsParamsCount-1, RB,<br>RE);<br>+ assert(reg >= RE);<br>+ }<br>+ }<br></blockquote>Is this only for assertion? Is this necessary?<br></blockquote>Yes. I've just moved this check from "if" statement,<br>since it<br>must be<br>true always.<br></blockquote>How do you think, do we need this assertion?<br></blockquote>I don't really think it is necessary :)<br><blockquote type="cite"><br>I didn't present refactor patch, just because main patch<br>didn't<br>get LGTM community and hypothetically may contain some<br>mistakes..<br></blockquote>LGTM otherwise.<br><br>Thanks,<br>Manman<br><blockquote type="cite"><br>-Stepan<br><br>Stepan Dyatkovskiy wrote:<br><blockquote type="cite">Manman Ren wrote:<br><blockquote type="cite"><br>Hi Stepan,<br><br>One high-level question: do we need to insert the wasted<br>record to<br>ByValRegs and later on skip those records?<br>It seems to me that we can only insert the records for<br>the<br>actual byval<br>parameters.<br>If that is true, we can remove addByValWastedRegs and<br>skipWastedByValRegs.<br></blockquote>Hm.. yes its reasonable. I'll try to fix.<br><br><blockquote type="cite"><br><blockquote type="cite">+<br>+ // "offset" value would be usefull if byval<br>parameter outsides<br>+ // the registers area. Note, in that case<br>nextInRegsParam()<br>will not<br>+ // skip any waste registers, since parameter<br>fillup<br>the whole<br>rest<br>+ // of registers set.<br>+ unsigned RegsUsed = RegEnd - RegBegin;<br>+ if (!CCInfo.nextInRegsParam() &&<br>Flags.getByValSize() ><br>4*RegsUsed )<br>+ offset = RegEnd - RegBegin;<br> }<br><br>- if (Flags.getByValSize() - 4*offset > 0) {<br>+ if (offset) {<br></blockquote><br>This still does not look right.<br>There are 3 cases:<br>If the byval parameter is in registers partially, we<br>should<br>create a<br>COPY_STRUCT_BYVAL with the correct offset;<br>If the byval parameter is all in registers, there is<br>no need<br>to create a<br>COPY_STRUCT_BYVAL;<br>If the byval parameter is all on stack, we should<br>create a<br>COPY_STRUCT_BYVAL with offset equal to 0.<br><br>When looking at the above code, offset is default to<br>zero, if<br>CurByValIdx >= ByValArgsCount (this byval parameter is<br>all on<br>stack),<br>offset will still be zero,<br>and we will not create a COPY_STRUCT_BYVAL.<br>The correct logic seems to be always updating offset<br>inside<br>the if and<br>check Flags.getByValSize() > 4*offset<br>if (CurByValIdx < ByValArgsCount) {<br> …<br> offset = RegEnd - RegBegin; //number of registers<br>occupied<br>by this<br>byval parameter<br> CCInfo.nextInRegsParam(); // go to the next byval<br>parameter that is<br>in registers partially or all<br>}<br>if (Flags.getByValSize() > 4*offset) {<br> // Part of the byval parameter is on stack.<br> ...<br></blockquote>Oh.. I missed third case. Then we keep first variant here.<br><br><blockquote type="cite"><blockquote type="cite">- if ((!State->isFirstByValRegValid()) &&<br>- (ARM::R0 <= reg) && (reg <= ARM::R3)) {<br>+ if ((ARM::R0 <= reg) && (reg <= ARM::R3)) {<br>+ // All registers reserved for byval parameters<br>should<br>be allocated.<br>+ // Check that first unallocated register is higher<br>that<br>last one<br>used<br>+ // for byval parameters.<br>+ {<br>+ unsigned InRegsParamsCount =<br>State->getInRegsParamsCount();<br>+ if (InRegsParamsCount) {<br>+ unsigned RB, RE;<br>+<br>State->getInRegsParamInfo(InRegsParamsCount-1, RB,<br>RE);<br>+ assert(reg >= RE);<br>+ }<br>+ }<br></blockquote>Is this only for assertion? Is this necessary?<br></blockquote>Yes. I've just moved this check from "if" statement,<br>since it<br>must be<br>true always.<br><br><blockquote type="cite"><blockquote type="cite">-void<br>-ARMTargetLowering::VarArgStyleRegisters(CCState<br>&CCInfo,<br>SelectionDAG<br>&DAG,<br>- DebugLoc dl,<br>SDValue &Chain,<br>- const Value<br>*OrigArg,<br>- unsigned<br>OffsetFromOrigArg,<br>- unsigned<br>ArgOffset,<br>- bool<br>ForceMutable)<br>const {<br>+// Return: The frame index registers were stored into.<br>+int<br>+ARMTargetLowering::StoreByValRegs(CCState &CCInfo,<br>SelectionDAG &DAG,<br>+ DebugLoc dl, SDValue<br>&Chain,<br>+ const Value *OrigArg,<br>+ unsigned<br>InRegsParamRecordIdx,<br>+ unsigned<br>OffsetFromOrigArg,<br>+ unsigned ArgOffset,<br>+ bool ForceMutable)<br>const {<br></blockquote><br>It would be much cleaner if we can move the<br>refactoring of<br>VarArgStyleRegisters to a separate patch.<br></blockquote>For sure.<br><br>P.S.: Today I had some troubles with electricity, so<br>I'll try<br>present<br>new patch tomorrow.<br><br>-Stepan.<br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><br><pr15293-2013-04-12.patch><br></blockquote><br></blockquote><br><stack-offset.c><VarArgsRegSaveSize-2013-04-16.patch><AAPCS-C.4-vs-VFP-2013-04-16.patch><br><br><br><br><br></blockquote><br></blockquote><br><AAPCS-C.4-vs-VFP-2013-04-17.patch><br></blockquote><br></blockquote><br><AAPCS-C.4-vs-VFP-2013-04-19.patch><br></blockquote><br></blockquote><br></blockquote><br></blockquote><br><StoreByValRegs-2013-04-17.patch><br></blockquote><br></blockquote><br></blockquote><br><StoreByValRegs-2013-04-28.patch><br></blockquote><br></blockquote><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote><br><pr15293-2013-05-01.patch><br></blockquote><br></blockquote><br><span><2013-05-02-AAPCS-ByVal-Structs-C4-C5-VFP.ll></span><span><2013-05-02-AAPCS-ByVal-Structs-C4-C5-VFP2.ll></span><span><MySandbox-test-c.4-c.5.ll></span><span><pr15293-2013-05-02.patch></span></div></blockquote></div><br></body></html>