Bug 129563 – DFG and FTL should specialize for and support CompareStrictEq over Misc (i.e. boolean, undefined, or null)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 225574 [details]
work in progress

[Filip Pizlo]

Created attachment 225584 [details]
the patch

[Filip Pizlo]

Created attachment 225586 [details]
the patch

[Filip Pizlo]

Created attachment 225595 [details]
the patch

[Filip Pizlo]

Created attachment 225596 [details]
the patch

[Geoffrey Garen]

Comment on attachment 225596 [details]
the patch

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:403
> +SpeculatedType leastUpperBoundOfStrictlyEquivalentSpeculations(SpeculatedType type)
> +{
> +    if (type & SpecInteger)
> +        type |= SpecInteger;
> +    if (type & SpecString)
> +        type |= SpecString;
> +    return type;
> +}

Maybe it's just Monday morning, but this function looks like a no-op. You test the input type for SpecInteger, and then set SpecInteger if and only if it's already set. Sam for SpecString. Can this be right?

> Source/JavaScriptCore/bytecode/SpeculatedType.h:79
> +static const SpeculatedType SpecMisc               = 0x30000000; // It's definitely either a boolean, Null, or Undefined.

Looks like the convention here is to capitalize "Boolean".

[Filip Pizlo]

Comment on attachment 225596 [details]
the patch

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

>> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:403
>> +}
> 
> Maybe it's just Monday morning, but this function looks like a no-op. You test the input type for SpecInteger, and then set SpecInteger if and only if it's already set. Sam for SpecString. Can this be right?

It's right.

SpecInteger is a bitmask of SpecInt32, SpecInt52, and SpecInt52AsDouble.  So, consider that 'type' may be just SpecInt32 (which is 0x00800000).  This function will turn it into SpecInt32 | SpecInt52 | SpecInt52AsDouble, which is 0x03800000.

Similarly for SpecString.  SpecString is a combo of SpecStringIdent and SpecStringVar.

>> Source/JavaScriptCore/bytecode/SpeculatedType.h:79
>> +static const SpeculatedType SpecMisc               = 0x30000000; // It's definitely either a boolean, Null, or Undefined.
> 
> Looks like the convention here is to capitalize "Boolean".

Will fix.

[Geoffrey Garen]

Comment on attachment 225596 [details]
the patch

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

r=me

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:405
> +bool speculationsCouldBeEqual(SpeculatedType a, SpeculatedType b)

I think "valuesCouldBeEqual" would be clearer here. The input is speculations, but the question we're asking is about the run-time values.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:421
> +    // If either side could be an object or not, then we could go down some weird
> +    // effectful path.

I think the effect you're talking about is that we'll do a value or string conversion on the object. Let's say that: "...then the equality comparison could call a toString or valueOf conversion, which could return anything".

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/165085

[Alexey Proskuryakov]

imported/w3c/html-templates/template-element/template-content.html started to crash with an assertion failure after this change:

Filip, are you available to take a look soon?

ASSERTION FAILED: needsTypeCheck(edge, typesPassedThrough)
/Volumes/Data/slave/mavericks-debug/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp(204) : void JSC::DFG::SpeculativeJIT::typeCheck(JSC::JSValueSource, JSC::DFG::Edge, SpeculatedType, MacroAssembler::Jump)
1   0x10b43b9b0 WTFCrash
2   0x10af93de8 JSC::DFG::SpeculativeJIT::typeCheck(JSC::JSValueSource, JSC::DFG::Edge, unsigned int, JSC::AbstractMacroAssembler<JSC::X86Assembler>::Jump)
3   0x10afab144 JSC::DFG::SpeculativeJIT::speculateMisc(JSC::DFG::Edge, JSC::JSValueRegs)
4   0x10afd99ae JSC::DFG::SpeculativeJIT::compileMiscStrictEq(JSC::DFG::Node*)
5   0x10afa682a JSC::DFG::SpeculativeJIT::compileStrictEq(JSC::DFG::Node*)
6   0x10afe3acf JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*)
7   0x10af99a74 JSC::DFG::SpeculativeJIT::compileCurrentBlock()
8   0x10af9a3dd JSC::DFG::SpeculativeJIT::compile()
9   0x10af2cfe4 JSC::DFG::JITCompiler::compileBody()

[WebKit Commit Bot]

Re-opened since this is blocked by bug 129729

[Filip Pizlo]

(In reply to comment #10)
> imported/w3c/html-templates/template-element/template-content.html started to crash with an assertion failure after this change:
> 
> Filip, are you available to take a look soon?
> 
> ASSERTION FAILED: needsTypeCheck(edge, typesPassedThrough)
> /Volumes/Data/slave/mavericks-debug/build/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp(204) : void JSC::DFG::SpeculativeJIT::typeCheck(JSC::JSValueSource, JSC::DFG::Edge, SpeculatedType, MacroAssembler::Jump)
> 1   0x10b43b9b0 WTFCrash
> 2   0x10af93de8 JSC::DFG::SpeculativeJIT::typeCheck(JSC::JSValueSource, JSC::DFG::Edge, unsigned int, JSC::AbstractMacroAssembler<JSC::X86Assembler>::Jump)
> 3   0x10afab144 JSC::DFG::SpeculativeJIT::speculateMisc(JSC::DFG::Edge, JSC::JSValueRegs)
> 4   0x10afd99ae JSC::DFG::SpeculativeJIT::compileMiscStrictEq(JSC::DFG::Node*)
> 5   0x10afa682a JSC::DFG::SpeculativeJIT::compileStrictEq(JSC::DFG::Node*)
> 6   0x10afe3acf JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*)
> 7   0x10af99a74 JSC::DFG::SpeculativeJIT::compileCurrentBlock()
> 8   0x10af9a3dd JSC::DFG::SpeculativeJIT::compile()
> 9   0x10af2cfe4 JSC::DFG::JITCompiler::compileBody()

Ugh, this was a stupid typo - speculateMisc() should have said DFG_TYPE_CHECK instead of typeCheck.  That's what the assertion is noticing.

I will reland with the fix shortly.

[Filip Pizlo]

Relanded in http://trac.webkit.org/changeset/165099