WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
69321
JITCodeGenerator should no longer have code that tries too hard to be both speculative and non-speculative
https://bugs.webkit.org/show_bug.cgi?id=69321
Summary
JITCodeGenerator should no longer have code that tries too hard to be both sp...
Filip Pizlo
Reported
2011-10-03 23:57:21 PDT
JITCodeGenerator used to be a superclass for NonSpeculativeJIT and SpeculativeJIT. Now the NonSpeculativeJIT is gone. There are some warts from this: - JITCodeGenerator has a m_isSpeculative field. This field is now always true. - JITCodeGenerator has a method to emit a speculation check, if we're in speculative mode. Code that needs speculation checks should probably just be moved to SpeculativeJIT. - JITCodeGenerator has an emitBranch method that tries to unify both speculative and non-speculative handling of branches. Now it should probably just be speculative.
Attachments
the patch
(17.44 KB, patch)
2011-10-03 23:59 PDT
,
Filip Pizlo
oliver
: review-
Details
Formatted Diff
Diff
the patch - address some of review
(17.62 KB, patch)
2011-10-04 13:50 PDT
,
Filip Pizlo
barraclough
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Filip Pizlo
Comment 1
2011-10-03 23:59:30 PDT
Created
attachment 109585
[details]
the patch
Oliver Hunt
Comment 2
2011-10-04 08:56:32 PDT
Comment on
attachment 109585
[details]
the patch 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. 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?
> 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)
> 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
> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:593 > + if (isKnownBoolean(node.child1())) {
no numeric fast case?
> 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
> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:626 > + if (!predictBoolean) {
This branch is always taken
Filip Pizlo
Comment 3
2011-10-04 12:44:09 PDT
(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.
Filip Pizlo
Comment 4
2011-10-04 13:50:51 PDT
Created
attachment 109676
[details]
the patch - address some of review The goal of this patch is to remove m_isSpeculative and speculationCheck() from JITCodeGenerator, since neither of those members belong there now that we don't have a non-speculative DFG JIT. To do that, it moves emitBranch() to SpeculativeJIT, and does not change it other than obvious clean-ups (like when you remove m_isSpeculative and assume that it's always true then some control flow changes).
Gavin Barraclough
Comment 5
2011-10-04 14:44:39 PDT
I'm a little confused, is speculationCheck not used in other places? assuming I'm missing something, r+.
Filip Pizlo
Comment 6
2011-10-04 15:01:43 PDT
(In reply to
comment #5
)
> I'm a little confused, is speculationCheck not used in other places? assuming I'm missing something, r+.
I'm referring to JITCodeGenerator::speculationCheck() which did something like: void JITCodeGenerator::speculationCheck(...) { ASSERT(m_isSpeculative); static_cast<SpeculativeJIT*>(this)->speculationCheck(...); }
Filip Pizlo
Comment 7
2011-10-04 16:02:15 PDT
Landed in
r96661
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug