[PATCH] Short-term workaround for hidden x86 float precision in calculateSpillWeightAndHint()

[Andrew Trick]

On Apr 21, 2014, at 9:39 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> The attached patch is a workaround that prevents hidden precision from
> changing results in x86 floats in calculateSpillWeightAndHint().
> 
> Applying my BlockFrequencyInfo patch causes two test failures on i386
> (see, e.g., r206704).  My patch changes the output of the pass, and
> somehow exposes this problem in calculateSpillWeightAndHint().  In
> the affected code:
> 
>    float hweight = Hint[hint] += weight;
>    if (TargetRegisterInfo::isPhysicalRegister(hint)) {
>      if (hweight > bestPhys && mri.isAllocatable(hint))
>        bestPhys = hweight, hintPhys = hint;
>    }
> 
> I converted `hweight` and `bestPhys` to APFloat and used
> APFloat::toString() to dump them.  In the problematic code path, they
> are both exactly equal to 1.  However, the comparison
> `hweight > bestPHys` gives `true` due to hidden precision in x86.
> Marking `hweight` as `volatile` forces it to be a stack variable
> (preventing hidden precision), so this comparison correctly gives
> `false` even on i386 (hack suggested by chapuni!).
> 
> This is an awkward workaround.  There seem to be 4 possible fixes
> here, so let me enumerate them:
> 
> 1. Mark `hweight` as `volatile` (attached patch).  This is a very
>    quick fix that unblocks my BlockFrequencyInfo work, but if adopted
>    should be revisited afterwards.  In particularly, I'll yell FIXME
>    somewhere and reference the *right* fix.

That’s fine to unblock your work.

> 2. Compare against a threshold, e.g.:
> 
>        hweight > bestPhys + 0.0001
> 
>    This is also a quick fix.  However, is it correct?  If we compare
>    2^-14 against 2^-15, we'll get the wrong result.
> 
> 3. Redesign spill weights to use integers.
> 
> 4. Change spill weights to use a soft-float.  I happen to have a
>    soft-float implementation lying around here somewhere…

The native float implementation of spill weights was always just a placeholder for doing something sensible (<rdar://problem/11938736> [PGO] Better spill weight model). I think the idea was that we would eventually come up with a fixed point representation for it—but that would add complexity. This seems like a good excuse to have something like PositiveFloat around--a convenient soft-float that gains efficiency and conveinence in exchange for ditching canonical form, normalization, and representing boundary conditions.

-Andy

> Thoughts?
> 
> Is this patch okay to commit for now?  What's the right longterm fix?
> 
> <CalcSpillWeights-volatile-float.patch>