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

[MAYUR PANDEY]

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>