| Summary: | [DFG] Should not fixup AnyIntUse in 32_64 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, saam | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2016-08-19 22:32:46 PDT
Created attachment 286560 [details]
Patch
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. Comment on attachment 286560 [details] Patch Clearing flags on attachment: 286560 Committed r204697: <http://trac.webkit.org/changeset/204697> All reviewed patches have been landed. Closing bug. Oops, DFGSpeculativeJIT64.cpp also has special path, it also needs some cares. Hmmmmm, webkit-bot is not working on IRC now... Committed r204698: <http://trac.webkit.org/changeset/204698> Roll out manually. I'll upload the patch with DFGSpeculativeJIT64.cpp fix. Created attachment 286572 [details]
Patch
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. Created attachment 286576 [details]
Patch
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. 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? (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? (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. Comment on attachment 286576 [details] Patch Clearing flags on attachment: 286576 Committed r204699: <http://trac.webkit.org/changeset/204699> All reviewed patches have been landed. Closing bug. |