Bug 129563

Summary: DFG and FTL should specialize for and support CompareStrictEq over Misc (i.e. boolean, undefined, or null)
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, ggaren, mark.lam, mhahnenberg, mmirman, msaboff, nrotem, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 129729    
Bug Blocks: 112840    
Attachments:
Description Flags
work in progress
none
the patch
none
the patch
none
the patch
none
the patch ggaren: review+

Description Filip Pizlo 2014-03-01 16:40:12 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2014-03-01 16:41:46 PST
Created attachment 225574 [details]
work in progress
Comment 2 Filip Pizlo 2014-03-01 20:09:33 PST
Created attachment 225584 [details]
the patch
Comment 3 Filip Pizlo 2014-03-01 20:38:32 PST
Created attachment 225586 [details]
the patch
Comment 4 Filip Pizlo 2014-03-02 08:21:18 PST
Created attachment 225595 [details]
the patch
Comment 5 Filip Pizlo 2014-03-02 08:24:04 PST
Created attachment 225596 [details]
the patch
Comment 6 Geoffrey Garen 2014-03-03 10:51:49 PST
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".
Comment 7 Filip Pizlo 2014-03-03 11:01:50 PST
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.
Comment 8 Geoffrey Garen 2014-03-03 13:00:16 PST
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".
Comment 9 Filip Pizlo 2014-03-04 16:58:36 PST
Landed in http://trac.webkit.org/changeset/165085
Comment 10 Alexey Proskuryakov 2014-03-04 22:11:17 PST
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()
Comment 11 WebKit Commit Bot 2014-03-04 23:16:25 PST
Re-opened since this is blocked by bug 129729
Comment 12 Filip Pizlo 2014-03-04 23:25:30 PST
(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.
Comment 13 Filip Pizlo 2014-03-04 23:35:41 PST
Relanded in http://trac.webkit.org/changeset/165099