Bug 129563

Created attachment 225574 [details] work in progress

Created attachment 225584 [details] the patch

Created attachment 225586 [details] the patch

Created attachment 225595 [details] the patch

Created attachment 225596 [details] the patch

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 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 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".

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

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()

Re-opened since this is blocked by bug 129729

(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.

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