[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


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109585|review?                     |review-
               Flag|                            |




--- Comment #2 from Oliver Hunt <oliver at apple.com>  2011-10-04 08:56:32 PST ---
(From update of attachment 109585)
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

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