[Webkit-unassigned] [Bug 67460] Add JSVALUE32_64 support to DFG JIT

Fri Sep 23 23:15:20 PDT 2011

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

--- Comment #22 from Filip Pizlo <fpizlo at apple.com>  2011-09-23 23:15:20 PST ---
(From update of attachment 108574)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.