[PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring

[Quentin Colombet]

Hi Mayur,

I’ve committed your patch:
Committed revision 205601.

I’ve updated the PR with that information too.

Thanks,
Quentin
On Apr 2, 2014, at 11:21 PM, MAYUR PANDEY <mayur.p at samsung.com> wrote:

> Hi Quentin,
> 
> Thanks a lot for your help and suggestions. I do not have commit access, so can you please commit on my behalf.
> 
> Thanks,
> Mayur
> 
> ------- Original Message -------
> Sender : Quentin Colombet<qcolombet at apple.com>
> Date : Apr 03, 2014 01:50 (GMT+09:00)
> Title : Re: [PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring
> 
> Hi Mayur,
> 
> 
> This looks good to me!
> 
> 
> Thanks,
> 
> -Quentin
> 
> 
> On Apr 2, 2014, at 7:16 AM, MAYUR PANDEY <mayur.p at samsung.com> wrote:
> 
> 
> Hi Quentin,
> 
> Modified the ragreedy-last-chance-recoloring.ll TC for the two cut-offs and modified the patch accordingly. Please review.
> 
> Thanks,
> Mayur
> 
> ------- Original Message -------
> Sender : Quentin Colombet<qcolombet at apple.com>
> Date : Apr 02, 2014 01:13 (GMT+09:00)
> Title : Re: [PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring
> 
> Hi Mayur,
> 
> 
> This generally LGTM.
> 
> 
> Would it be possible to add a test case?
> (You can add another RUN line in the existing last chance recoloring test where you tweak the options.)
> 
> 
> 
> 
> Thanks,
> -Quentin
> 
> 
> On Apr 1, 2014, at 6:32 AM, MAYUR PANDEY <mayur.p at samsung.com> wrote:
> 
> 
> Hi Quentin,
> 
> Thanks for reviewing the patch and giving your suggestions. Based on your comments, I have modified the patch. 
> 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. 
> 
> Thanks,
> Mayur
> ------- Original Message -------
> Sender : Quentin Colombet<qcolombet at apple.com>
> Date : Mar 29, 2014 04:14 (GMT+09:00)
> Title : Re: [PATCH]: Bug 18747 - Add an option to allow an exhaustive search during last chance recoloring
> 
> Hi Mayur,
> 
> Thanks for working on this.
> 
> The general structure looks good to me, but we have to make a few fixes:
> #1
> +    // Both lcr-max-depth and lcr-max-interf encountered
> +    CO_Depth_Interf
> +  };
> You do not need this one, just assign 0 to None, 1 to Depth, and 2 to Interf.
> Then, we will use them as bit position in a byte.
> 
> #2
> +  struct CutoffInfo {
> +    CutOffStage Stage;
> +    void setCutOffInfo(CutOffStage state = CO_None) {
> +      Stage = state;
> +    }
> +  }CO_Info;
> 
> CO_Info can simply be an uint8_t.
> Note: if you want to hide the bit masking tricks, you can encapsulate those in the struct you’d proposed.
> 
> #2.1 BTW, I am not sure using _ in a variable name is common practice in the LLVM code base.
> 
> This will solve this bug:
>     DEBUG(dbgs() << "Early abort: too many interferences.\n");
> +      CO_Info.setCutOffInfo(CO_Interf);
>     return false;
> 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.
> 
> #3
> With the new proposal, this simply becomes:
> CO_Info |= CO_Interf
> 
> #4
> Likewise, this code sequence:
> +    // set the cutoffstage
> +    if (CO_Info.Stage == CO_Interf)
> +      CO_Info.setCutOffInfo(CO_Depth_Interf);
> +    else
> +      CO_Info.setCutOffInfo(CO_Depth);
> becomes:
> CO_Info |= CO_Depth
> 
> +  CO_Info.setCutOffInfo();
> => CO_Info = CO_None;
> 
> #5
> +  if (Reg == ~0U && (CO_Info.Stage != CO_None)) {
> +    if (CO_Info.Stage == CO_Depth)
> +      report_fatal_error("register allocation failed: maximum depth for recoloring reached");
> +    else if (CO_Info.Stage == CO_Interf)
> +      report_fatal_error("register allocation failed: maximum interference for recoloring reached");
> +    else
> +      report_fatal_error("register allocation failed: maximum interference and depth for recoloring reached");
> +  }
> =>
> +  if (Reg == ~0U && (CO_Info != CO_None)) {
> +    uint8_t COEncountered = CO_Info & (CO_Depth | CO_Interf);
> +    if (COEncountered == CO_Depth)
> +      report_fatal_error("register allocation failed: maximum depth for recoloring reached");
> +    else if (COEncountered == CO_Interf)
> +      report_fatal_error("register allocation failed: maximum interference for recoloring reached");
> +    else if (COEncountered == (CO_Depth | CO_Interf))
> +      report_fatal_error("register allocation failed: maximum interference and depth for recoloring reached");
> +  }
> 
> #6
> 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).
> 
> #7
> The error messages may be more specific.
> 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.
> That said, this could be improved later and is not a blocker for your contribution :).
> 
> Thanks again!
> -Quentin
> 
> On Mar 28, 2014, at 2:14 AM, MAYUR PANDEY wrote:
> 
> 
> 
> Hi,
> 
> Attached is the patch for solving the 1st thing in the Bug 18747:
> 1. When the assignment fails because of the early cutoffs, this should be reported instead of the current obscur “ran out of registers”.
> the discussion can be checked at http://llvm.org/bugs/show_bug.cgi?id=18747
> 
> Please review the patch.
> 
> Thanks,
> Mayur
> <regrecolorcutoff.patch><regrecolorcutoff.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140403/cd9a3e86/attachment.html>