Bug 161029 – [DFG] Should not fixup AnyIntUse in 32_64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Currently DFGFixup for ProfileType node fixes up AnyIntUse edge filter.
But AnyIntUse is only allowed in 64bit DFG. Should not do that.

[Yusuke Suzuki]

Created attachment 286560 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 286560 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1461
>              if (typeSet->doesTypeConformTo(TypeAnyInt)) {

I think that logging AnyInt in 32bit TypeSet should be OK, since I don't think it is good that the type profiler tells different types between 32bit and 64bit.

[WebKit Commit Bot]

Comment on attachment 286560 [details]
Patch

Clearing flags on attachment: 286560

Committed r204697: <http://trac.webkit.org/changeset/204697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Oops, DFGSpeculativeJIT64.cpp also has special path, it also needs some cares.
Hmmmmm, webkit-bot is not working on IRC now...

[Yusuke Suzuki]

Committed r204698: <http://trac.webkit.org/changeset/204698>

[Yusuke Suzuki]

Roll out manually.
I'll upload the patch with DFGSpeculativeJIT64.cpp fix.

[Yusuke Suzuki]

Created attachment 286572 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 286572 [details]
Patch

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

Take 2. DFGSpeculativeJIT64 needs some cares.

> Source/JavaScriptCore/ChangeLog:22
> +        I think introducing hierarchy into RuntimeType is better (e.g. TypeNumber includes TypeAnyInt).

If it is necessary, I think it should be done in the separate patch. (But i don't know it is really necessary, since DFGSpeculativeJIT32_64 does not have this fast path).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5125
> +        // Be careful! If the last seen type is TypeNumber, it does not include the values classified into TypeAnyInt.

This is the updated part.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:-5143
> -            jumpToEnd.append(m_jit.branchTest64(MacroAssembler::NonZero, valueGPR, GPRInfo::tagTypeNumberRegister));

This is incorrect shortcut. TypeNumber does not include TypeAnyInt.

[Yusuke Suzuki]

Created attachment 286576 [details]
Patch

[Saam Barati]

Me and Yusuke spoke on IRC, basically, the original patch was correct,
however, one of the tests wasn't completely correct because it assumes
that TypeNumber being set also means TypeInteger being set. We should probably
get to that point, and have a more coherent story for this behavior.

Currently, TypeInt means only integers were seen on the TypeSet.
And, TypeNumber means either only Doubles were seen, or both
Doubles and Integers were seen. So once TypeNumber is seen in the TypeSet,
we implicitly mean an Integer could have seen seen. This means that
we can't distinguish between Integer or Double at that point.
We just know some type of number was seen.

[Saam Barati]

Comment on attachment 286576 [details]
Patch

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

> JSTests/typeProfiler/int52-dfg.js:17
> +assert(types.instructionTypeSet.primitiveTypeNames.indexOf(T.Integer) !== -1, "Primitive type names should contain 'Integer'");

Won't this be viewed as a Double in 32-bit environments?

[Saam Barati]

(In reply to comment #12)
> Comment on attachment 286576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286576&action=review
> 
> > JSTests/typeProfiler/int52-dfg.js:17
> > +assert(types.instructionTypeSet.primitiveTypeNames.indexOf(T.Integer) !== -1, "Primitive type names should contain 'Integer'");
> 
> Won't this be viewed as a Double in 32-bit environments?

More specifically, won't the TypeSet see TypeNumber at that point on 32-bit platforms?

[Yusuke Suzuki]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 286576 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=286576&action=review
> > 
> > > JSTests/typeProfiler/int52-dfg.js:17
> > > +assert(types.instructionTypeSet.primitiveTypeNames.indexOf(T.Integer) !== -1, "Primitive type names should contain 'Integer'");
> > 
> > Won't this be viewed as a Double in 32-bit environments?
> 
> More specifically, won't the TypeSet see TypeNumber at that point on 32-bit
> platforms?

Yup. We use JSValue::isAnyInt() in runtimeTypeForValue to produce TypeAnyInt.
And JSValue::isAnyInt() returns true if the value is Int52 in all the platform.

[WebKit Commit Bot]

Comment on attachment 286576 [details]
Patch

Clearing flags on attachment: 286576

Committed r204699: <http://trac.webkit.org/changeset/204699>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.