Bug 67460 – Add JSVALUE32_64 support to DFG JIT

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yuqiang Xian]

The meta bug to track JSVALUE32_64 support in DFG JIT

We're trying to add JSVALUE32_64 support to DFG JIT. Yet we're not sure if there are any ongoing efforts for this. It would be helpful if any similar work or plan being conducted can be exposed. If none then this may be the initiation of the work. 
We also welcome any suggestions, for example on the development model: whether we need a separate branch or we just post patches here when ready? And any other related discussions. Thanks.

[Gavin Barraclough]

Hi Yuqiang,

We're definitely interested in seeing the DFG JIT ported to all platforms.  In order to do so without the code duplication involved in the old JIT we're looking at options to unify the two value representations.  The JSVALUE32_64 representation seems to offer a significant advantage over JSVALUE64, since this does not require double values to be boxed/unboxed (which we currently do via integer registers, resulting in a int->fp move).  We hope to be able to get x86-64 working on JSVALUE32_64 at a progression over JSVALUE64.  This is what I'm looking at right now, and I have a preliminary cut of this working, but working badly - not ready for review, a regression, and with a couple of unacceptable hacks in the progress.  If I can fix the remaining issues, I should have a patch up soon.

If we are successful in unifying on one value representations then we can simply switch the DFG JIT over, rather than having to make the code support both representations.

hope this plan sounds good,
G.

[Yuqiang Xian]

Gavin,

Thanks a lot for the valuable information. Yes it looks great to have only one value representation in DFG JIT, which as you pointed out, avoids code duplication to support two different representations.

If possible, could you please share the rough timeline of the readiness of the patch to have x86-64 using JSVALUE32_64 in DFG JIT? While it's fine if it's not available yet.

Actually we've been doing the IA32 platform enabling work based on current DFG JIT code base, of course using the JSVALUE32_64 representation. I guess this won't be conflict with your work (but not sure), except that we may need to re-base our work on the new code if your changes are committed. What do you think, any suggestions?

Thanks.

[Yuqiang Xian]

Hi,

I noticed big changes recently with the introduction of "tierd compilation" in JSC, which supports switching between speculative DFG JIT and the old JIT.
Will this become the future direction of JSC? And will the non-speculative JIT be abandoned (and the estimated timeline)? I ask this because it will help us make right choices in our development work on enabling DFG JIT on 32-bit platforms.

Thanks a lot.

[Filip Pizlo]

(In reply to comment #3)
> Hi,
> 
> I noticed big changes recently with the introduction of "tierd compilation" in JSC, which supports switching between speculative DFG JIT and the old JIT.
> Will this become the future direction of JSC? And will the non-speculative JIT be abandoned (and the estimated timeline)? I ask this because it will help us make right choices in our development work on enabling DFG JIT on 32-bit platforms.
> 
> Thanks a lot.

Hi!

Tiered compilation is definitely the future direction of JSC.  This means that the non-speculative JIT will definitely be removed.

Performing this switch is predicated upon tiered compilation being a pure win on all benchmark suites, and having zero major regressions in individual benchmarks (though we expect small regressions on very short-running benchmarks).

Right now this is already true when running on the command-line (jsc), but is not yet true in browser.  These in-browser bugs are being fixed as we speak.

My optimistic estimate is that tiered compilation will be turned on in trunk in the next week.  My pessimistic estimate is that this will happen in two weeks.  The non-speculative JIT, and all code for static prediction and speculation, will likely be removed shortly thereafter.

-Filip

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #3)
> > Hi,
> > 
> > I noticed big changes recently with the introduction of "tierd compilation" in JSC, which supports switching between speculative DFG JIT and the old JIT.
> > Will this become the future direction of JSC? And will the non-speculative JIT be abandoned (and the estimated timeline)? I ask this because it will help us make right choices in our development work on enabling DFG JIT on 32-bit platforms.
> > 
> > Thanks a lot.
> 
> Hi!
> 
> Tiered compilation is definitely the future direction of JSC.  This means that the non-speculative JIT will definitely be removed.
> 
> Performing this switch is predicated upon tiered compilation being a pure win on all benchmark suites, and having zero major regressions in individual benchmarks (though we expect small regressions on very short-running benchmarks).
> 
> Right now this is already true when running on the command-line (jsc), but is not yet true in browser.  These in-browser bugs are being fixed as we speak.
> 
> My optimistic estimate is that tiered compilation will be turned on in trunk in the next week.  My pessimistic estimate is that this will happen in two weeks.  The non-speculative JIT, and all code for static prediction and speculation, will likely be removed shortly thereafter.
> 
> -Filip

Note that our performance target has been met as of today.  As soon as all of the bots are happy we will enable tiered compilation in trunk by default (for platforms that support DFG) and proceed with removing the non-speculative JIT and static prediction logic.

https://bugs.webkit.org/show_bug.cgi?id=68136

[Yuqiang Xian]

Thank you Filip. We're working on the new code base with tiered compilation now.

[Gavin Barraclough]

Hi Yuqiang,

We're currently no longer predicating bringing up the DFG JIT on JSVALUE32_64 on the experiment to move JSVALUE64 platforms over to JSVALUE32_64.  As such we're going to want to start to make progress towards getting JSVALUE32_64 working.  We understand you are interested in this too, so to minimize conflicts we'll try to make incremental changes so that our work in progress is visible.

cheers,
G.

[Yuqiang Xian]

Created attachment 108267 [details]
initial patch for JSVALUE32_64 support in DFG JIT

Hi Gavin,

Glad to know that you're going to enable JSVALUE32_64 support in DFG JIT for 32-bit platforms. Yes we're very interested in this and it's exactly what we've been trying to do. Actually now we're heavily debugging our code and I was thinking to release the code for your reviews after it gets mature enough. But since you're going to work on this I think it would be better to share the initial code with you to have a first glance, though current code has many bugs (known and unknown) and is dirty - for example having some code duplications with the JSVALUE64 code, and some "TODO"s spreaded. It would be great if you can have a look at it and share your comments. Please note that this is not a request for a formal review as it doesn't reach such stage yet. The code is based on revision 95501 (1~2 days behind current trunk).

The code is able to run some benchmark cases in SunSpider, and we really see some improvements on the cases where DFG is truly triggered. The results will be attached later. Currently the 32-bit DFG switch can be turned on/off in the Efl port build.

Thanks a lot. It's really exciting to know that you're also going to do this.

-Yuqiang

[Yuqiang Xian]

Created attachment 108268 [details]
initial performance result on (incomplete) SunSpider for 32-bit DFG

[Filip Pizlo]

(In reply to comment #9)
> Created an attachment (id=108268) [details]
> initial performance result on (incomplete) SunSpider for 32-bit DFG

What are the results on Kraken and V8?  SunSpider does not benefit from DFG nearly as much.

[Yuqiang Xian]

(In reply to comment #10)

> What are the results on Kraken and V8?  SunSpider does not benefit from DFG nearly as much.

Hi Filip, thanks for your interest. We're trying to enable all the SunSpider cases at current stage (still 7 cases failed to run), and our plan is after that we'll look at Kraken and V8, and yes we expect more benefit on those two benchmarks.

[Gavin Barraclough]

Hi Yuqiang,

Wow, I hadn't realized quite how far you were along with this, this is really awesome.  The sooner we can get this in the tree the better.  The main thing to do is to get this cleaned up (remove #if 0'ed & commented out code, etc) & up for review, & I'll be happy to review as soon as you're ready.  If there are any final problems to address we can potentially land disabled, and then I'd be happy to help parallelize fixing any last issues.  Let me know if there is anything I can do.

I think this duplicates more code than we probably need to (e.g. arith ops that operate purely on SpeculateInteger / SpeculateDouble operands), but we could refactor to unify that between JSVALUE64 & JSVALUE32_64 once this is in the tree.

cheers,
G.

[Yuqiang Xian]

Gavin,

Thanks a lot for your suggestion. Yes we'll perform an update with latest trunk and merge our changes, and then we'll clean-up current code and upload it for you to review. We think there must be some problems not addressed for now and we'll try to give out a summary when uploading the code. It's really great that you're willing to help on resolving the problems. Hopefully the tided code can be uploaded in one or two days.

Thanks, -Yuqiang

[Yuqiang Xian]

Created attachment 108452 [details]
initial patch for JSVALUE32_64 support in DFG JIT

Hi Gavin,

The cleaned-up code is uploaded. Can you please help review it? Thank you. It's still not as clean as it could be, for example there're unnecessary code duplications and some known issues as I mentioned in the ChangeLog. But as you suggested, we could fix them step by step.

Thanks a lot.
-Yuqiang

[WebKit Review Bot]

Attachment 108452 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/CMakeL..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1424:  movsd_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:527:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:533:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 108452 [details]
initial patch for JSVALUE32_64 support in DFG JIT

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

The biggest problem with this patch is the amount of code duplicated, but let's get this into the tree so we can work together on better sharing code with JSVALUE64.  There are a bunch of issues I've raised above, but they are all things I'm happy to see addressed in follow on patches.  Otherwise all looks good, the code looks clean, let's get this landed.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:-123
> -        if (spillMe != InvalidVirtualRegister)

These extra loops over the gprs are a little unfortunate.  We could probably ask the GenerationInfo if it has a paired register?

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:259
> +        if (!info.needsSpill() || (!(info.registerFormat() & DataFormatJS) && matchAny(info.gpr(), info.gpr(), exclude, exclude2)) || ((info.registerFormat() & DataFormatJS) && matchAny(info.tagGPR(), info.payloadGPR(), exclude, exclude2)))

I think we should break:
    (!(info.registerFormat() & DataFormatJS) && matchAny(info.gpr(), info.gpr(), exclude, exclude2)) || ((info.registerFormat() & DataFormatJS) && matchAny(info.tagGPR(), info.payloadGPR(), exclude, exclude2))
out into a separate helper function (since I think the same check is used again below).  Also I think this could be written cleanly with a ?: .

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:1145
> +#elif USE(JSVALUE32_64)

This should probably really be CPU(X86), other 32_64 platforms will probably pass arguments in registers.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:1157
> +        m_jit.push(JITCompiler::TrustedImm32(reinterpret_cast<int>(pointer)));

We probably don't want to be pushing anything in these methods.  In the JIT I think we leave a buffer of stack space preallocated to write arguments out to for calls out from JIT code, and differing numbers of pushes will cause misalignment of the stack (Mac OS X requires the stack be aligned to  a 16 byte boundary prior to the call instruction).

> Source/JavaScriptCore/dfg/DFGJITCompilerInlineMethods.h:160
> +// FIXME : Should we also add this map feature to DFG JIT?

No, I don't think so - this was a simple 1/2 entry register allocation from the old JIT.  The DFG JIT already has more comprehensive register allocation which can be tailored to our needs, this should probably just be removed.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:263
> +#if CPU(X86_64)

This ordering inconsistency is unfortunate, we should try to fix this at some point.  I think the JIT code has always left sufficient space free on the stack that we can just store above the existing arguments without tramping anything?

[Yuqiang Xian]

Created attachment 108574 [details]
patch addressing Gavin's comments

Gavin, 

Thanks a lot for the review. Here's the new patch addressing your comments, though some of issues are left to be handled, for example the unnecessary code duplications and the arguments passing mechanism.

BTW, there's also another fix which included the recently added "BitVector.cpp" in the wtf CMakeLists file to make the build succeed.

[WebKit Review Bot]

Attachment 108574 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/CMakeL..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1439:  movsd_rm is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:527:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/dfg/DFGOperations.cpp:533:  The parameter name """" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yuqiang Xian]

(In reply to comment #16)
> (From update of attachment 108452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108452&action=review
> 
> The biggest problem with this patch is the amount of code duplicated, but let's get this into the tree so we can work together on better sharing code with JSVALUE64.  There are a bunch of issues I've raised above, but they are all things I'm happy to see addressed in follow on patches.  Otherwise all looks good, the code looks clean, let's get this landed.

Yes. Unnecessary code duplications need to be resolved in the near future. 

> 
> > Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:-123
> > -        if (spillMe != InvalidVirtualRegister)
> 
> These extra loops over the gprs are a little unfortunate.  We could probably ask the GenerationInfo if it has a paired register?

Done.

> 
> > Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:259
> > +        if (!info.needsSpill() || (!(info.registerFormat() & DataFormatJS) && matchAny(info.gpr(), info.gpr(), exclude, exclude2)) || ((info.registerFormat() & DataFormatJS) && matchAny(info.tagGPR(), info.payloadGPR(), exclude, exclude2)))
> 
> I think we should break:
>     (!(info.registerFormat() & DataFormatJS) && matchAny(info.gpr(), info.gpr(), exclude, exclude2)) || ((info.registerFormat() & DataFormatJS) && matchAny(info.tagGPR(), info.payloadGPR(), exclude, exclude2))
> out into a separate helper function (since I think the same check is used again below).  Also I think this could be written cleanly with a ?: .

Done.

> 
> > Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:1145
> > +#elif USE(JSVALUE32_64)
> 
> This should probably really be CPU(X86), other 32_64 platforms will probably pass arguments in registers.

Done.

> 
> > Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:1157
> > +        m_jit.push(JITCompiler::TrustedImm32(reinterpret_cast<int>(pointer)));
> 
> We probably don't want to be pushing anything in these methods.  In the JIT I think we leave a buffer of stack space preallocated to write arguments out to for calls out from JIT code, and differing numbers of pushes will cause misalignment of the stack (Mac OS X requires the stack be aligned to  a 16 byte boundary prior to the call instruction).

Yes, we can address this later.

> 
> > Source/JavaScriptCore/dfg/DFGJITCompilerInlineMethods.h:160
> > +// FIXME : Should we also add this map feature to DFG JIT?
> 
> No, I don't think so - this was a simple 1/2 entry register allocation from the old JIT.  The DFG JIT already has more comprehensive register allocation which can be tailored to our needs, this should probably just be removed.

Done.

> 
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:263
> > +#if CPU(X86_64)
> 
> This ordering inconsistency is unfortunate, we should try to fix this at some point.  I think the JIT code has always left sufficient space free on the stack that we can just store above the existing arguments without tramping anything?

Agree. We can address this later.

[Gavin Barraclough]

Fixed in r95902.

[Gavin Barraclough]

Oooops, missed the cake files, landed in 95905.

[Filip Pizlo]

Comment on attachment 108574 [details]
patch addressing Gavin's comments

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

This is really fantastic!  Here are some comments on possible future improvements.

> Source/JavaScriptCore/dfg/DFGGenerationInfo.h:135
> +        return to != DataFormatInteger && to != DataFormatCell;

It should never be the case that needDataFormatConversion() is called with from == DataFormatCell and to == DataFormatInteger, since needDataFormatConversion() is for cases where DFG changed the format but not the underlying value.  Going between Integer and Cell could only happen if we changed the value, as well.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:66
> +            else {
> +                ASSERT(isJSConstant(nodeIndex));
> +                JSValue jsValue = valueOfJSConstant(nodeIndex);
> +                m_jit.move(MacroAssembler::Imm32(jsValue.payload()), gpr);

Should this ever be reached?  If we're trying to fill an integer and the constant is not an integer, then something likely went horribly wrong.  So this is either dead code or it's just wrong.  I think we have the same bug in the JSVALUE64 code.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:316
> +GPRReg JITCodeGenerator::fillStorage(NodeIndex nodeIndex)
> +{

It would be great to figure out some way of ensuring that ops that deal with DataFormatStorage share code between 32_64 and 64.  In both cases, Storage should use one GPR.  The only difference is that loading data from storage should go to two registers in 32_64 and one register in 64.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:345
> +void JITCodeGenerator::useChildren(Node& node)
> +{

This is one of those super-obvious cases where the code should be common between JSVALUE64 and 32_64.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:372
> +bool JITCodeGenerator::isStrictInt32(NodeIndex nodeIndex)
> +{

These isFooBar() methods should be identical in the two value representations.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:1088
> +void JITCodeGenerator::nonSpeculativeInstanceOf(Node& node)
> +{
> +    // FIXME: Currently we flush all registers as the number of available registers
> +    // does not meet our requirement.

The non-speculative version of this code seems like a great candidate for coming up with a cheap out-of-line stub.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:1179
> +    m_jit.loadPtr(JITCompiler::Address(basePayloadGPR, JSObject::offsetOfPropertyStorage()), resultPayloadGPR);
> +    JITCompiler::DataLabelCompact loadWithPatch = m_jit.loadPtrWithCompactAddressOffsetPatch(JITCompiler::Address(resultPayloadGPR, 0), resultPayloadGPR);
> +    m_jit.move(TrustedImm32(JSValue::CellTag), resultTagGPR);

Is this right?  When we load a value from the heap, we have no guarantee that it will be a cell!  It could be an integer.  Or a double.  Or something else.  We really need to patchable loads - one for the payload and one for the tag.  The offset between the two "should" be a constant.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator32_64.cpp:1351
> +    writeBarrier(basePayloadGPR, valueTagGPR, valueIndex, WriteBarrierForPropertyAccess, scratchGPR);
> +
> +    m_jit.loadPtr(JITCompiler::Address(basePayloadGPR, JSObject::offsetOfPropertyStorage()), scratchGPR);
> +    JITCompiler::DataLabel32 storeWithPatch = m_jit.storePtrWithAddressOffsetPatch(valuePayloadGPR, JITCompiler::Address(scratchGPR, 0));
> +
> +    JITCompiler::Jump done = m_jit.jump();

Again, you're not storing the tag.  That's a bug.

> Source/JavaScriptCore/dfg/DFGJITCompiler32_64.cpp:88
> +void JITCompiler::exitSpeculativeWithOSR(const OSRExit& exit, SpeculationRecovery* recovery, Vector<BytecodeAndMachineOffset>& decodedCodeMap)
> +{

We really need to ensure that this code is as common as possible between the two representations, since this code is super tricky and really hard to test.

> Source/JavaScriptCore/dfg/DFGJITCompiler32_64.cpp:123
> +        case BooleanSpeculationCheck:
> +            break;

Perhaps this is ASSERT_NOT_REACHED for 32_64?

> Source/JavaScriptCore/dfg/DFGJITCompiler32_64.cpp:212
> +    EncodedJSValue* scratchBuffer = static_cast<EncodedJSValue*>(globalData()->scratchBufferForSize(sizeof(EncodedJSValue) * (numberOfPoisonedVirtualRegisters + ((numberOfDisplacedVirtualRegisters * 2) <= GPRInfo::numberOfRegisters ? 0 : numberOfDisplacedVirtualRegisters))));
> +
> +    // From here on, the code assumes that it is profitable to maximize the distance
> +    // between when something is computed and when it is stored.
> +    
> +    // 4) Perform all reboxing of integers.
> +    //    Currently we don't rebox for JSValue32_64.
> +    
> +    // 5) Dump all non-poisoned GPRs. For poisoned GPRs, save them into the scratch storage.
> +    //    Note that GPRs do not have a fast change (like haveFPRs) because we expect that
> +    //    most OSR failure points will have at least one GPR that needs to be dumped.

Prior to this part, the code is identical between value representations.

> Source/JavaScriptCore/dfg/DFGJITCompiler32_64.cpp:402
> +    // 11) Adjust the old JIT's execute counter. Since we are exiting OSR, we know
> +    //     that all new calls into this code will go to the new JIT, so the execute
> +    //     counter only affects call frames that performed OSR exit and call frames
> +    //     that were still executing the old JIT at the time of another call frame's
> +    //     OSR exit. We want to ensure that the following is true:

From here on, the code looks identical to 64-bit.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:698
> +    case BitAnd:
> +    case BitOr:
> +    case BitXor:
> +        if (isInt32Constant(node.child1())) {
> +            SpeculateIntegerOperand op2(this, node.child2());
> +            GPRTemporary result(this, op2);
> +
> +            bitOp(op, valueOfInt32Constant(node.child1()), op2.gpr(), result.gpr());
> +
> +            integerResult(result.gpr(), m_compileIndex);

It's great to see that for many of these ops - anything that speculates integer or double - it appears that the code is identical between 32_64 and 64.  It seems that code that is different is the exception rather than the rule.

[Yuqiang Xian]

Filip, thanks for the comments. Yes we have many things to do, especially the code cleaning/refactoring to remove the unnecessary duplications.
Also, thanks for pointing out the bugs. I have filed another bug #68755 to address the bugs regarding GetById/PutById. Yes you're right that we need to load/store and repatch both tag and payload, while I don't assume the offset between the two patched loads/stores to be constant in my current fix. Your comments are highly appreciated.

Thanks, -Yuqiang

[Yuqiang Xian]

bug #68794 has been created for refactoring the JSVALUE32_64 DFG code to remove unnecessary code duplications.

[Yuqiang Xian]

The status was incorrectly marked by accident.