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.
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.
Created attachment 238400 [details] work in progress convert to Check nodes in FixupPhase if possible.
Created attachment 238736 [details] patch generated machine code now works.
Created attachment 238987 [details] patch
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.
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, ...
> >> 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.
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.
Created attachment 239017 [details] patch Same as above, but actually builds.
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
(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.
(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; }
Created attachment 239036 [details] patch - Removed the unnecessary class in DFGSlowPathGenerator by explicitly handling the NoResult in DFGSpeculativeJIT::callOperation
Created attachment 239037 [details] patch (Uploaded wrong diff above).
landed in: http://trac.webkit.org/changeset/174167