[PATCH] RegAlloc: Account for a variable entry block frequency

[Duncan P. N. Exon Smith]

Thanks for the review!

On 2014 Apr 7, at 20:47, Manman Ren <manman.ren at gmail.com> wrote:

>    unsigned calculateRegionSplitCost(LiveInterval &VirtReg,
>                                      AllocationOrder &Order,
> -                                    BlockFrequency &BestCost,
> +                                    const BlockFrequency &AlternateCost,
>  unsigned RAGreedy::calculateRegionSplitCost(LiveInterval &VirtReg,
>                                              AllocationOrder &Order,
> -                                            BlockFrequency &BestCost,
> +                                            const BlockFrequency &AlternateCost,
>                                              unsigned &NumCands,
>                                              bool IgnoreCSR) {
> +  BlockFrequency BestCost = AlternateCost;
> --> The above should be separated (is there a reason for these changes?)

Without this change, the argument (sometimes CSRCost) gets modified
and reset to zero.  This becomes a problem when CSRCost is initialized
only once per MachineFunction.  This change prevents that.

Since no callers check it after the call, a non-const reference was
strange API anyway.

I've split it into a separate patch below.

> +  uint64_t FixedEntry = 1 << 14;
> +  if (ActualEntry < FixedEntry)
> +    CSRCost *= BranchProbability(ActualEntry, FixedEntry);
> +  else if (ActualEntry <= FixedEntry * 16)
> +    // Invert the fraction and divide.
> +    CSRCost /= BranchProbability(FixedEntry, ActualEntry);
> +  else
> +    // Can't use BranchProbability in general, since it takes 32-bit numbers.
> +    // This path is faster anyway, albeit less precise.
> +    CSRCost = CSRCost.getFrequency() * (ActualEntry >> 14);
> --> Are we trying to be precise when possible with this if statement? Why a fixed 16 in the 2nd condition?

I was prematurely optimizing, and only guaranteeing 4 bits of
precision so that it "usually" took the fast path.  The new patch is
far less magical:

  - it uses UINT32_MAX instead of FixedEntry*16 (probably no slower in
    practice), and

  - it uses ActualEntry/FixedEntry instead of ActualEntry>>14 (exact
    same math, but more human-readable).

> Thanks,
> Manman

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-RegAlloc-Stop-modifying-the-argument-to-calculateSpl.patch
Type: application/octet-stream
Size: 1849 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140407/8a97c7c4/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-RegAlloc-Account-for-a-variable-entry-block-frequenc.patch
Type: application/octet-stream
Size: 7454 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140407/8a97c7c4/attachment-0001.obj>