Runtime: double.IsNaN suboptimal codegen for constants

[EgorBo]

I have some IsNaN checks in my code and I expected JIT to remove them if I use a compile-time constants, e.g.

[MethodImpl(MethodImplOptions.AggressiveOptimization)]
static bool Test()
{
    return double.IsNaN(5);
}

Currently generates:

G_M30276_IG01:
       push     rax
       vzeroupper 

G_M30276_IG02:
       vmovsd   xmm0, qword ptr [reloc @RWD00]
       vmovsd   qword ptr [rsp], xmm0
       mov      rax, qword ptr [rsp]
       mov      rdx, 0xD1FFAB1E
       and      rax, rdx
       mov      rdx, 0xD1FFAB1E
       cmp      rax, rdx
       setg     al
       movzx    rax, al

G_M30276_IG03:
       add      rsp, 8
       ret      
RWD00  dq   4014000000000000h
; Total bytes of code: 58

Despite the fact double.IsNaN is inlineable and pretty simple.
So I expected it to be just:

       mov      eax, 1
       retu

category:cq
theme:inlining
skill-level:expert
cost:medium

[mikedn]

return double.IsNaN(5);

Why would someone write such code? Or is the real code producing constants in a less obvious way (e.g. inlining)?

[EgorBo]

@mikedn it's just an example, the original code was like

if (double.IsNaN(x))
    ThrowHelper.ThrowCanNotBeNan();

(and the whole method is inlineable so x becomes a constant)

[mikedn]

OK, I'll take a look. I happen to have a prototype that might help with this.

[EgorBo]

I noticed that TimeSpan.FromX(x) APIs are quite frequently used with constants, e.g.

var timeout = TimeSpan.FromSeconds(30);

in such cases it should be just a few movs.

So I went to TimeSpan.Interval, replaced exceptions with ThrowHelpers, the second (overflow) check was correctly optimized away but not the IsNaN one.

[mikedn]

With some VN quick and dirty changes on top of another change I had the JIT promptly generates xor eax, eax for IsNaN(5)

https://github.com/mikedn/coreclr/commits/fp-int-reinterpret

[EgorBo]

@mikedn nice!
Tested your changes locally (after I made TimeSpan.Interval inlineable):

static TimeSpan GetTimeout()
{
    return TimeSpan.FromMinutes(5);
}

generates:

G_M40445_IG01:
G_M40445_IG02:
       mov      eax, 0xD1FFAB1E
G_M40445_IG03:
       ret   

(not sure it should be inlineable when input is not a const value)

[mikedn]

(not sure it should be inlineable when input is not a const value)

Probably not, unless it's used in some loop that's hotter than a nuclear rector core :)

[tannergooding]

I don't see why it shouldn't be inlineable. On most hardware a nan check is trivial (generally a single comparison -- on x86 this is ucomisd/ucomiss). There is quite a bit of code that utilizes IsNaN (including other Math functions, number formatting, TimeSpan, and WinForms/WPF) and ideally we would just handle this optimally.

[mikedn]

He's talking about TimeSpan.Interval: https://github.com/dotnet/coreclr/blob/d2bba5450c03d5f8e6d0d6428ae59a79321364ca/src/System.Private.CoreLib/shared/System/TimeSpan.cs#L203-L211

Even if it is changed to use ThrowHelper it's still pretty large (despite really being a multiply and a cast).

[EgorBo]

It needs something like [InlineIfConst(nameof(value)] similar to the [NotNullIfNotNull(...)] 🙂 or a list of special cases in the inliner.

[gfoidl]

[InlineIfConst(nameof(value)]

I'd like the JIT handle this within it's heuristics, and not us via an attribute.
If it's const input, be more aggressive with inlining or that like.

[EgorBo]

[InlineIfConst(nameof(value)]

I'd like the JIT handle this within it's heuristics, and not us via an attribute.
If it's const input, be more aggressive with inlining or that like.

I see inliner already takes it into account:

Inline candidate has const arg that feeds a conditional.  Multiplier increased to 4.

but still even with removed IsNaN check it fails to inline:

        [MethodImpl(MethodImplOptions.AggressiveOptimization)]
        public TimeSpan Test(double value, double scale)
        {
            return Interval(10, 10);
        }

        [MethodImpl(MethodImplOptions.AggressiveOptimization)]
        private static TimeSpan Interval(double value, double scale)
        {
            double millis = value * scale;
            if ((millis > long.MaxValue) || (millis < long.MinValue))
                ThrowHelper.ThrowOE();
            return new TimeSpan((long)millis);
        }

Inline candidate has an arg that feeds a constant test.  Multiplier increased to 1.
Inline candidate has const arg that feeds a conditional.  Multiplier increased to 4.
Inline candidate callsite is boring.  Multiplier increased to 5.3.
calleeNativeSizeEstimate=829
callsiteNativeSizeEstimate=115
benefit multiplier=5.3
threshold=609
Native estimate for function size exceeds threshold for inlining 82.9 > 60.9 (multiplier = 5.3)

(maybe some multipliers need to be adjusted, e.g. a const arg feeds a constant test, etc
cc @AndyAyersMS

[AndyAyersMS]

The current inline heuristics have a number of shortcomings. I'm generally of the opinion that tweaking what's there is generally not the right approach.

The various "... feeds constant" observations are based on a simplistic scan of the inlinee IL (done during fgFindJumpTargets). The stack modelling done during this scan is pretty sketchy. I think the deductions it makes in the above example are actually wrong.

As for what I consider the right, approach, this writeup may prove instructive. While that investigation lead to more questions than answers, it suggested that that the code size impact of inlines is amenable to modelling, even when the inliner is restricted to the crude kinds of inlinee observations we're likely to be able to afford in a jit.

[omariom]

This spill

       vmovsd   qword ptr [rsp], xmm0
       mov      rax, qword ptr [rsp]

can be cured with changing https://github.com/dotnet/coreclr/blob/82ed48cfbfc478c71890d60f993037d1208fb3c4/src/System.Private.CoreLib/shared/System/BitConverter.cs#L452 to
C# [MethodImpl(MethodImplOptions.AggressiveInlining)] public static unsafe long DoubleToInt64Bits(double value) { if (Sse2.X64.IsSupported) { return Sse2.X64.ConvertToInt64(Vector128.CreateScalarUnsafe(value).AsInt64()); } return *((long*)&value); }

Does it smell?.. 👃

[omariom]

There is quite a bit of code that utilizes IsNaN (including ..WPF)..

WPF has its own IsNaN which is now slower than double's one :)

[tannergooding]

can be cured with changing
Does its smell?.. 👃

It's either that or updating the JIT to explicitly understand that a reinterpret cast of double to long can be movq (and float to int can be movd).

A benefit from the JIT explicitly understanding it is that it can explicitly elide things if it is already in memory and just read the bits directly. Where-as the above code will still force a load and store.

Likewise could be said for IsNaN. We could just do a ucomiss comparison and check for the parity flag; but the JIT understanding this check properly would mean it could be slightly more efficient.

[EgorBo]

Closing this (I guess it's fixed by the x != x implementation for double.IsNaN
Codegen for the snippet in the description:

; Method Program:Test():bool
G_M31255_IG01:
                        ;; bbWeight=1    PerfScore 0.00

G_M31255_IG02:
       xor      eax, eax
                        ;; bbWeight=1    PerfScore 0.25

G_M31255_IG03:
       ret      
                        ;; bbWeight=1    PerfScore 1.00
; Total bytes of code: 3