<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Mayur,<div><br></div><div>I’ve committed your patch:</div><div><div style="margin: 0px; font-size: 11px; font-family: Menlo;">Committed revision 205601.</div></div><div><br></div><div>I’ve updated the PR with that information too.</div><div><br></div><div>Thanks,</div><div>Quentin<br><div><div>On Apr 2, 2014, at 11:21 PM, MAYUR PANDEY <<a href="mailto:mayur.p@samsung.com">mayur.p@samsung.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hi Quentin,<br><br>Thanks a lot for your help and suggestions. I do not have commit access, so can you please commit on my behalf.<br><br>Thanks,<br>Mayur<br><br>------- Original Message -------<br>Sender : Quentin Colombet<<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>><br>Date : Apr 03, 2014 01:50 (GMT+09:00)<br>Title : Re: [PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring<br><br>Hi Mayur,<br><br><br>This looks good to me!<br><br><br>Thanks,<br><br>-Quentin<br><br><br>On Apr 2, 2014, at 7:16 AM, MAYUR PANDEY <<a href="mailto:mayur.p@samsung.com">mayur.p@samsung.com</a>> wrote:<br><br><br>Hi Quentin,<br><br>Modified the ragreedy-last-chance-recoloring.ll TC for the two cut-offs and modified the patch accordingly. Please review.<br><br>Thanks,<br>Mayur<br><br>------- Original Message -------<br>Sender : Quentin Colombet<<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>><br>Date : Apr 02, 2014 01:13 (GMT+09:00)<br>Title : Re: [PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring<br><br>Hi Mayur,<br><br><br>This generally LGTM.<br><br><br>Would it be possible to add a test case?<br>(You can add another RUN line in the existing last chance recoloring test where you tweak the options.)<br><br><br><br><br>Thanks,<br>-Quentin<br><br><br>On Apr 1, 2014, at 6:32 AM, MAYUR PANDEY <<a href="mailto:mayur.p@samsung.com">mayur.p@samsung.com</a>> wrote:<br><br><br>Hi Quentin,<br><br>Thanks for reviewing the patch and giving your suggestions. Based on your comments, I have modified the patch. <br>For #7 The error messages may be more specific. : we can give more specific error when we implement -fexhaustive-register-search for the same. We can specify the user the current max-depth and max-interf and also give this option in error msg for exhaustive search. <br><br>Thanks,<br>Mayur<br>------- Original Message -------<br>Sender : Quentin Colombet<<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>><br>Date : Mar 29, 2014 04:14 (GMT+09:00)<br>Title : Re: [PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring<br><br>Hi Mayur,<br><br>Thanks for working on this.<br><br>The general structure looks good to me, but we have to make a few fixes:<br>#1<br>+ // Both lcr-max-depth and lcr-max-interf encountered<br>+ CO_Depth_Interf<br>+ };<br>You do not need this one, just assign 0 to None, 1 to Depth, and 2 to Interf.<br>Then, we will use them as bit position in a byte.<br><br>#2<br>+ struct CutoffInfo {<br>+ CutOffStage Stage;<br>+ void setCutOffInfo(CutOffStage state = CO_None) {<br>+ Stage = state;<br>+ }<br>+ }CO_Info;<br><br>CO_Info can simply be an uint8_t.<br>Note: if you want to hide the bit masking tricks, you can encapsulate those in the struct you’d proposed.<br><br>#2.1 BTW, I am not sure using _ in a variable name is common practice in the LLVM code base.<br><br>This will solve this bug:<br> DEBUG(dbgs() << "Early abort: too many interferences.\n");<br>+ CO_Info.setCutOffInfo(CO_Interf);<br> return false;<br>At this point, this is possible that the depth case was encountered during the same session, thus this setting is not accurate and we would have to play the same if/else trick as you did for the depth case.<br><br>#3<br>With the new proposal, this simply becomes:<br>CO_Info |= CO_Interf<br><br>#4<br>Likewise, this code sequence:<br>+ // set the cutoffstage<br>+ if (CO_Info.Stage == CO_Interf)<br>+ CO_Info.setCutOffInfo(CO_Depth_Interf);<br>+ else<br>+ CO_Info.setCutOffInfo(CO_Depth);<br>becomes:<br>CO_Info |= CO_Depth<br><br>+ CO_Info.setCutOffInfo();<br>=> CO_Info = CO_None;<br><br>#5<br>+ if (Reg == ~0U && (CO_Info.Stage != CO_None)) {<br>+ if (CO_Info.Stage == CO_Depth)<br>+ report_fatal_error("register allocation failed: maximum depth for recoloring reached");<br>+ else if (CO_Info.Stage == CO_Interf)<br>+ report_fatal_error("register allocation failed: maximum interference for recoloring reached");<br>+ else<br>+ report_fatal_error("register allocation failed: maximum interference and depth for recoloring reached");<br>+ }<br>=><br>+ if (Reg == ~0U && (CO_Info != CO_None)) {<br>+ uint8_t COEncountered = CO_Info & (CO_Depth | CO_Interf);<br>+ if (COEncountered == CO_Depth)<br>+ report_fatal_error("register allocation failed: maximum depth for recoloring reached");<br>+ else if (COEncountered == CO_Interf)<br>+ report_fatal_error("register allocation failed: maximum interference for recoloring reached");<br>+ else if (COEncountered == (CO_Depth | CO_Interf))<br>+ report_fatal_error("register allocation failed: maximum interference and depth for recoloring reached");<br>+ }<br><br>#6<br>Instead of using report_fatal_error, use LLVMContext::diagnose or LLVMContext::emitError. You should be able to access the LLVMContext through the MF field (maybe with one indirection: getFunction).<br><br>#7<br>The error messages may be more specific.<br>I admit I do not have a good idea now for the exact messages, but mentioning last chance recoloring and the limits used for the related cut-off (maybe the name of the related option and its actual value) may help to decide what to do.<br>That said, this could be improved later and is not a blocker for your contribution :).<br><br>Thanks again!<br>-Quentin<br><br>On Mar 28, 2014, at 2:14 AM, MAYUR PANDEY wrote:<br><br><br><br>Hi,<br><br>Attached is the patch for solving the 1st thing in the Bug 18747:<br>1. When the assignment fails because of the early cutoffs, this should be reported instead of the current obscur “ran out of registers”.<br>the discussion can be checked at <a href="http://llvm.org/bugs/show_bug.cgi?id=18747">http://llvm.org/bugs/show_bug.cgi?id=18747</a><br><br>Please review the patch.<br><br>Thanks,<br>Mayur<br><regrecolorcutoff.patch><regrecolorcutoff.patch></blockquote></div><br></div></body></html>