Bug 136712 – Support the type profiler in the DFG

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

op_profile_type is not currently supported by the DFG.

This patch will support it in the most naive way, and then in later patch(es), optimizations will be made.

[Saam Barati]

Created attachment 238394 [details]
work in progress

Here is the initial implementation of just passing op_profile_type through the DFG in the simplest way possible.
I'm still not sure why the generated assembly is failing, any help from another pair of eyes would be appreciated.

[Saam Barati]

Created attachment 238400 [details]
work in progress

convert to Check nodes in FixupPhase if possible.

[Saam Barati]

Created attachment 238736 [details]
patch

generated machine code now works.

[Saam Barati]

Created attachment 238987 [details]
patch

[Filip Pizlo]

Comment on attachment 238987 [details]
patch

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.

[Saam Barati]

Comment on attachment 238987 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238987&action=review

>> 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.

It doesn't, doesTypeConformTo(T) will make sure that its internal bit string has no bits set outside the bits of T. Then the additional individual checks makes sure that both bits are set.

>> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:164
>> +
> 
> Why didn't CallResultAndNoArgumentsSlowPathGenerator with the result argument being "NoResult" work?

I didn't try this, actually. I should, it seems like it should work.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4937
>> +            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.

Yeah, I think this is the right solution. And, I'm going to also have the internal data structure represent this explicitly.

So the enum in TypeSet should be:
...
    TypeNumber = 0x20 | 0x10,
...

[Saam Barati]

> >> Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:164
> >> +
> > 
> > Why didn't CallResultAndNoArgumentsSlowPathGenerator with the result argument being "NoResult" work?
> 
> I didn't try this, actually. I should, it seems like it should work.

I wasn't thinking properly about this when I responded. I'm pretty sure I tried this, and the problem was that the DFG operation I define isn't of the correct type. I would have to declare a DFG operation that would have a non void return value. 

I'll double check to confirm this.

[Saam Barati]

Created attachment 239016 [details]
patch

- I left the additional class for slow path call w/ no arguments and no result type. It doesn't work
  by just passing in NoResult to the existing class because then the operation it calls is expected to 
  have a return type. I guess this can also be done by having a bogus return type in the operation, 
  but that seems a bit hacky to me. What do you think?

- I made the necessary changes to reflect that TypeInteger is contained in TypeNumber.
   This includes fixing the inlined type check when emitting code, and also the convert to check
   for NumberUse in the Fixup phase. This NumberUse Check is now generated more eagerly because
   I removed a few checks that were in place when TypeMachineInt wasn't contained in TypeNumber.

- I also decided to use "isInt32Speculation(prediction)" instead of "!(prediction & ~SpecInt32)" because it seems more consistent with the other checks of this nature in the Fixup phase.

[Saam Barati]

Created attachment 239017 [details]
patch

Same as above, but actually builds.

[Filip Pizlo]

Comment on attachment 239017 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239017&action=review

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1128
> +                if (isInt32Speculation(node->child1()->prediction()))

node->child1()->shouldSpeculateInt32() is the shortest way to say this

[Filip Pizlo]

(In reply to comment #8)
> Created an attachment (id=239016) [details]
> patch
> 
> - I left the additional class for slow path call w/ no arguments and no result type. It doesn't work
>   by just passing in NoResult to the existing class because then the operation it calls is expected to 
>   have a return type. I guess this can also be done by having a bogus return type in the operation, 
>   but that seems a bit hacky to me. What do you think?

I don't understand this point.  What error do you get?

We have *many* operations with void return type.  This should work.  If it doesn't, we should fix it; adding a new class to work around it is probably not the right solution.

> 
> - I made the necessary changes to reflect that TypeInteger is contained in TypeNumber.
>    This includes fixing the inlined type check when emitting code, and also the convert to check
>    for NumberUse in the Fixup phase. This NumberUse Check is now generated more eagerly because
>    I removed a few checks that were in place when TypeMachineInt wasn't contained in TypeNumber.
> 
> - I also decided to use "isInt32Speculation(prediction)" instead of "!(prediction & ~SpecInt32)" because it seems more consistent with the other checks of this nature in the Fixup phase.

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #8)
> > Created an attachment (id=239016) [details] [details]
> > patch
> > 
> > - I left the additional class for slow path call w/ no arguments and no result type. It doesn't work
> >   by just passing in NoResult to the existing class because then the operation it calls is expected to 
> >   have a return type. I guess this can also be done by having a bogus return type in the operation, 
> >   but that seems a bit hacky to me. What do you think?
> 
> I don't understand this point.  What error do you get?
> 
> We have *many* operations with void return type.  This should work.  If it doesn't, we should fix it; adding a new class to work around it is probably not the right solution.

Here's what you need.  Revert the change to DFGSpeculativeJIT64.cpp, it's just there to test if this compiles and to demonstrate the syntax.



Index: dfg/DFGSpeculativeJIT.h
===================================================================
--- dfg/DFGSpeculativeJIT.h	(revision 174092)
+++ dfg/DFGSpeculativeJIT.h	(working copy)
@@ -1087,6 +1087,12 @@
         return appendCallWithExceptionCheckSetResult(operation, result);
     }
 
+    JITCompiler::Call callOperation(V_JITOperation_E operation)
+    {
+        m_jit.setupArgumentsExecState();
+        return appendCallWithExceptionCheck(operation);
+    }
+
     JITCompiler::Call callOperation(V_JITOperation_EC operation, GPRReg arg1)
     {
         m_jit.setupArgumentsWithExecState(arg1);
@@ -1148,6 +1154,11 @@
         return appendCallWithExceptionCheckSetResult(operation, result);
     }
 
+    template<typename FunctionType>
+    JITCompiler::Call callOperation(FunctionType operation, NoResultTag)
+    {
+        return callOperation(operation);
+    }
     template<typename FunctionType, typename ArgumentType1>
     JITCompiler::Call callOperation(FunctionType operation, NoResultTag, ArgumentType1 arg1)
     {
Index: dfg/DFGSpeculativeJIT64.cpp
===================================================================
--- dfg/DFGSpeculativeJIT64.cpp	(revision 174092)
+++ dfg/DFGSpeculativeJIT64.cpp	(working copy)
@@ -2094,6 +2094,10 @@
         else
             callOperation(operationValueAdd, result.gpr(), op1GPR, op2GPR);
         
+        addSlowPathGenerator(
+            slowPathCall(
+                m_jit.jump(), this, operationVMHandleException, NoResult));
+        
         jsValueResult(result.gpr(), node);
         break;
     }

[Saam Barati]

Created attachment 239036 [details]
patch

- Removed the unnecessary class in DFGSlowPathGenerator by explicitly handling the NoResult in DFGSpeculativeJIT::callOperation

[Saam Barati]

Created attachment 239037 [details]
patch

(Uploaded wrong diff above).

[Saam Barati]

landed in: http://trac.webkit.org/changeset/174167