[Webkit-unassigned] [Bug 69321] JITCodeGenerator should no longer have code that tries too hard to be both speculative and non-speculative

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=69321





--- Comment #3 from Filip Pizlo <fpizlo at apple.com>  2011-10-04 12:44:09 PST ---
(In reply to comment #2)
> (From update of attachment 109585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109585&action=review
> 
> While i realize this patch is only trying to simplify the emitBranch logic, it seems sad that we don't emit code for object checks, etc inline when the profiles tell us we're looking at objects.

Actually, it's trying to eliminate speculative code from the DFGJITCodeGenerator, and eliminate code in DFGJITCodeGenerator that tries to be generic generic over m_isSpeculative.  emitBranch() was simply moved, almost entirely untouched, from DFGJITCodeGenerator into DFGSpeculativeJIT.

> 
> r- though because the 64bit version of emitBranch has weird control flow layout
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:487
> > +    // FIXME: Add fast cases for known Boolean!
> 
> Boolean implies boxed boolean to me -- do you mean boxed or unboxed here?

Not my comment, it was already there.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:496
> > +    use(node.child1());
> 
> What does this do? (unrelated to this particular patch, i just don't think i've seen use() before)

I didn't add it in this patch, it was already there.

It simply marks to the reg alloc that the value associated with this node does not have to be kept alive if we spill/fill.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:502
> > +    JITCompiler::Jump fastPath = m_jit.branch32(JITCompiler::Equal, valueTagGPR, JITCompiler::TrustedImm32(JSValue::Int32Tag));
> > +    JITCompiler::Jump slowPath = m_jit.branch32(JITCompiler::NotEqual, valueTagGPR, JITCompiler::TrustedImm32(JSValue::BooleanTag));
> 
> It would be nice if we could rework our encoding to make this work with a single branch, but again unrelated to this patch

That's not my code; I think our friend at Intel wrote that.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:593
> > +    if (isKnownBoolean(node.child1())) {
> 
> no numeric fast case?

There has never been one in this code.  I have a separate bug for that: https://bugs.webkit.org/show_bug.cgi?id=69322

My plan was to put that up for review once emitBranch() was moved to SpeculativeJIT.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:621
> > +        if (predictBoolean) {
> > +            addBranch(m_jit.branchPtr(MacroAssembler::Equal, valueGPR, MacroAssembler::ImmPtr(JSValue::encode(jsBoolean(false)))), notTaken);
> > +            addBranch(m_jit.branchPtr(MacroAssembler::Equal, valueGPR, MacroAssembler::ImmPtr(JSValue::encode(jsBoolean(true)))), taken);
> > +        }
> > +        
> > +        if (predictBoolean) {
> > +            speculationCheck(m_jit.jump());
> > +            value.use();
> 
> Why are these in two separate blocks

That's a screw-up.  Previously the second block tested for m_isSpeculative.  Now it doesn't, so I guess they should be merged and….

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:626
> > +            if (!predictBoolean) {
> 
> This branch is always taken

This should be killed.

Bottom line, I'm happy to simply just do this as one patch (see https://bugs.webkit.org/show_bug.cgi?id=69322), but I wanted to have a separate patch that gets rid of:

JITCodeGenerator::m_isSpeculative  (it's always true)
JITCodeGenerator::speculationCheck  (it's a horrible hack from the days when the DFG had two JITs)

and to do that I have to move emitBranch() to SpeculativeJIT, since that's the most egregious use of the two above things, which only existed because previously emitBranch() had to be in JITCodeGenerator to avoid code duplication.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.