<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Apr 7, 2014 at 8:25 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the review!<br>
<div class=""><br>
On 2014 Apr 7, at 20:47, Manman Ren <<a href="mailto:manman.ren@gmail.com">manman.ren@gmail.com</a>> wrote:<br>
<br>
>    unsigned calculateRegionSplitCost(LiveInterval &VirtReg,<br>
>                                      AllocationOrder &Order,<br>
> -                                    BlockFrequency &BestCost,<br>
> +                                    const BlockFrequency &AlternateCost,<br>
>  unsigned RAGreedy::calculateRegionSplitCost(LiveInterval &VirtReg,<br>
>                                              AllocationOrder &Order,<br>
> -                                            BlockFrequency &BestCost,<br>
> +                                            const BlockFrequency &AlternateCost,<br>
>                                              unsigned &NumCands,<br>
>                                              bool IgnoreCSR) {<br>
> +  BlockFrequency BestCost = AlternateCost;<br>
> --> The above should be separated (is there a reason for these changes?)<br>
<br>
</div>Without this change, the argument (sometimes CSRCost) gets modified<br>
and reset to zero.  This becomes a problem when CSRCost is initialized<br>
only once per MachineFunction.  This change prevents that.<br></blockquote><div>Got it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Since no callers check it after the call, a non-const reference was<br>
strange API anyway.<br></blockquote><div><br></div><div>Currently no caller uses BestCost, but the argument BestCost passes in the best cost</div><div>so far and it returns the best cost afterwards.</div><div><br></div><div>
I am okay with the change. Please commit,</div><div><br></div><div>Thanks,</div><div>Manman</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
I've split it into a separate patch below. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
> +  uint64_t FixedEntry = 1 << 14;<br>
> +  if (ActualEntry < FixedEntry)<br>
> +    CSRCost *= BranchProbability(ActualEntry, FixedEntry);<br>
> +  else if (ActualEntry <= FixedEntry * 16)<br>
> +    // Invert the fraction and divide.<br>
> +    CSRCost /= BranchProbability(FixedEntry, ActualEntry);<br>
> +  else<br>
> +    // Can't use BranchProbability in general, since it takes 32-bit numbers.<br>
> +    // This path is faster anyway, albeit less precise.<br>
> +    CSRCost = CSRCost.getFrequency() * (ActualEntry >> 14);<br>
> --> Are we trying to be precise when possible with this if statement? Why a fixed 16 in the 2nd condition?<br>
<br>
</div>I was prematurely optimizing, and only guaranteeing 4 bits of<br>
precision so that it "usually" took the fast path.  The new patch is<br>
far less magical:<br>
<br>
  - it uses UINT32_MAX instead of FixedEntry*16 (probably no slower in<br>
    practice), and<br>
<br>
  - it uses ActualEntry/FixedEntry instead of ActualEntry>>14 (exact<br>
    same math, but more human-readable).<br>
<br>
> Thanks,<br>
> Manman<br>
<br>
</blockquote></div><br></div></div>