[webkit-reviews] review denied: [Bug 69321] JITCodeGenerator should no longer have code that tries too hard to be both speculative and non-speculative : [Attachment 109585] the patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 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

Attachment 109585: the patch
https://bugs.webkit.org/attachment.cgi?id=109585&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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