[webkit-reviews] review denied: [Bug 136712] Support the type profiler in the DFG : [Attachment 238987] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Saam Barati <saambarati1 at gmail.com>'s
request for review:
Bug 136712: Support the type profiler in the DFG
https://bugs.webkit.org/show_bug.cgi?id=136712

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238987&action=review


See comments.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1129
> +		   if (node->child1()->prediction() & SpecInt32)
> +		       fixEdge<Int32Use>(node->child1());

This is wrong.	You're saying I'll check if it's an Int32 if it could have been
an Int32.  Instead, you want to say: I'll check if it's an Int32 if that's the
only thing it could have been.

The boolean logic would be "!(prediction & ~SpecInt32)".

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1132
> +	       } else if (typeSet->doesTypeConformTo(TypeNumber |
TypeMachineInt) && seenTypes & TypeNumber && seenTypes & TypeMachineInt) {

I would put parentheses around "seenTypes & TypeNumber" and "seenTypes &
TypeMachineInt".  Also, that boolean math smells like it might have the same
problem as the SpecInt32 problem above.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1141
> +	       } else if (typeSet->doesTypeConformTo(TypeUndefined | TypeNull)
&& seenTypes & TypeUndefined && seenTypes & TypeNull) {

Ditto.

> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:164
> +template<typename JumpType, typename FunctionType>
> +class CallNoResultAndNoArgumentsSlowPathGenerator
> +    : public CallSlowPathGenerator<JumpType, FunctionType, GPRReg> {
> +public:
> +    CallNoResultAndNoArgumentsSlowPathGenerator(
> +	   JumpType from, SpeculativeJIT* jit, FunctionType function,
> +	   SpillRegistersMode spillMode)
> +	   : CallSlowPathGenerator<JumpType, FunctionType, GPRReg>(
> +	       from, jit, function, spillMode, InvalidGPRReg)
> +    {
> +    }
> +    
> +protected:
> +    virtual void generateInternal(SpeculativeJIT* jit) override
> +    {
> +	   this->setUp(jit);
> +	   this->recordCall(jit->callOperation(this->m_function));
> +	   this->tearDown(jit);
> +    }
> +};
> +

Why didn't CallResultAndNoArgumentsSlowPathGenerator with the result argument
being "NoResult" work?

> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:361
> +template<typename JumpType, typename FunctionType>
> +inline PassOwnPtr<SlowPathGenerator> slowPathCall(
> +    JumpType from, SpeculativeJIT* jit, FunctionType function,
SpillRegistersMode spillMode = NeedToSpill)
> +{
> +    return adoptPtr(
> +	   new CallNoResultAndNoArgumentsSlowPathGenerator<
> +	       JumpType, FunctionType>(
> +		   from, jit, function, spillMode));
> +}
> +

See above - I think this shouldn't have been necessary.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4937
> +	   else if (cachedTypeLocation->m_lastSeenType == TypeNumber) {
> +	       // A JSValue is a double when it's a number and it's NOT an
int32.
> +	       MacroAssembler::Jump definitelyNotDouble =
m_jit.branchTest64(MacroAssembler::Zero, valueGPR,
GPRInfo::tagTypeNumberRegister);
> +	       JITCompiler::Jump notInt32 =
m_jit.branch64(MacroAssembler::Below, valueGPR,
GPRInfo::tagTypeNumberRegister);
> +	       jumpToEnd.append(notInt32);
> +	       definitelyNotDouble.link(&m_jit);

I really don't like this because it's just incompatible with how the DFG works.
 The DFG can coerce an int32 to a double behind the scenes if it believes that
this value could also be a double.  So, I think this is just unsound.

You should allow your type profiling to treat Double and Double|Int as
equivalent things.