Bug 228146 – speculateNeitherDoubleNorStringNorHeapBigInt should only have a single JSType branch

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

speculateNeitherDoubleNorStringNorHeapBigInt should only have a single JSType branch

[Keith Miller]

Created attachment 433934 [details]
Patch

[Keith Miller]

Comment on attachment 433934 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792
> +    DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt), m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType }));

It seems like this should be SpecAnyString but maybe I was missing something?

[Keith Miller]

Comment on attachment 433934 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792
>> +    DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt), m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType }));
> 
> It seems like this should be SpecAnyString but maybe I was missing something?

Nvm, I confused myself, I was thinking that SpecString independent from SpecStringIdent and SpecStringVar...

[Robin Morisset]

Comment on attachment 433934 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:7
> +

Maybe add some small comment here explaining your trick for people scanning through the Changelog.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792
>> +    DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt), m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType }));
> 
> It seems like this should be SpecAnyString but maybe I was missing something?

SpecAnyString does not exist, SpecString = SpecStringIdent | SpecStringVar is the union type.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-19012
> -        return m_out.equal(

I think I would have kept this version of isType intact, and called it from the JSTypeRange version when range.last == range.first, but I'm ok with your approach even if I find it less intuitive.

[Keith Miller]

Comment on attachment 433934 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:7
>> +
> 
> Maybe add some small comment here explaining your trick for people scanning through the Changelog.

Sure.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-19012
>> -        return m_out.equal(
> 
> I think I would have kept this version of isType intact, and called it from the JSTypeRange version when range.last == range.first, but I'm ok with your approach even if I find it less intuitive.

I just followed how the AssemblyHelpers version did it for consistency. I don't have strong feelings either way though.

[Keith Miller]

Created attachment 433942 [details]
Patch for landing

[EWS]

Committed r280149 (239846@main): <https://commits.webkit.org/239846@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433942 [details].

[Radar WebKit Bug Importer]

<rdar://problem/80906151>