ASSERT in speculateMachineInt on 32-bit platforms

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

Patch on the way.

[Mark Hahnenberg]

Created attachment 223064 [details]
Patch

[Geoffrey Garen]

Comment on attachment 223064 [details]
Patch

r=me

[Filip Pizlo]

Comment on attachment 223064 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223064&action=review

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:139
>              if (type == SpecInt52AsDouble)
> -                type = SpecInt52;
> +                type = enableInt52() ? SpecInt52 : SpecDouble;

I'm pretty sure this is wrong and will result in worse optimizations on 32-bit.  In particular, you're claiming that if something was representable as an in52 (i.e. it's non-fractional and not-NaN and not infinity), then it's SpecDouble, which means "this value can be an integer, or a real, or infinity, or NaN, etc".  So, this is not the right fix.

You should have instead just made this say:

if (type == SpecInt52AsDouble && enableInt52())
    type = SpecInt52;

[Mark Hahnenberg]

> You should have instead just made this say:
> 
> if (type == SpecInt52AsDouble && enableInt52())
>     type = SpecInt52;

But what happens when !enableInt52()? Is it valid for the speculated type to be SpecInt52AsDouble on 32-bit platforms?

[Filip Pizlo]

(In reply to comment #4)
> > You should have instead just made this say:
> > 
> > if (type == SpecInt52AsDouble && enableInt52())
> >     type = SpecInt52;
> 
> But what happens when !enableInt52()? Is it valid for the speculated type to be SpecInt52AsDouble on 32-bit platforms?

Yes.

[Mark Hahnenberg]

Created attachment 223135 [details]
Patch

[Mark Hahnenberg]

Committed r163391: <http://trac.webkit.org/changeset/163391>