[Webkit-unassigned] [Bug 69314] DFG should inline Array.push and Array.pop

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 3 20:01:30 PDT 2011


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


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ggaren at apple.com




--- Comment #7 from Geoffrey Garen <ggaren at apple.com>  2011-10-03 20:01:30 PST ---
Looks good overall, but I found some potential problems.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1284
> +        speculationCheck(m_jit.branch32(MacroAssembler::Above, storageLengthGPR, TrustedImm32(0x7ffffffe)));

Why is 0x7ffffffe special? I think we only need to guard unsigned overflow here. Could this just turn into a jo after the increment of storageLengthGPR?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1346
> +        MacroAssembler::Jump notHole = m_jit.branchTestPtr(MacroAssembler::NonZero, valueTagGPR);
> +        MacroAssembler::Jump holeCase = m_jit.branchTestPtr(MacroAssembler::Zero, valuePayloadGPR);

The whole value test in 32_64 should test the tag for equality to JSValue::EmptyValueTag. I think this is one more branch than necessary, and not correct.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1351
> +        m_jit.store32(storageLengthGPR, MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::ScalePtr, OBJECT_OFFSETOF(ArrayStorage, m_vector[0]) + OBJECT_OFFSETOF(JSValue, u.asBits.tag)));
> +        m_jit.store32(storageLengthGPR, MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::ScalePtr, OBJECT_OFFSETOF(ArrayStorage, m_vector[0]) + OBJECT_OFFSETOF(JSValue, u.asBits.payload)));

I think you're trying to leave JSValue() behind in the popped location. Storing zero is not the right way to do that. (See comment above.)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:1371
> +        silentSpillAllRegisters(valueTagGPR, valuePayloadGPR);
> +        m_jit.push(baseGPR);
> +        m_jit.push(GPRInfo::callFrameRegister);
> +        appendCallWithExceptionCheck(operationArrayPop);
> +        setupResults(valueTagGPR, valuePayloadGPR);
> +        silentFillAllRegisters(valueTagGPR, valuePayloadGPR);

Is inlining pop on a discontiguous array worth it? I'd expect the operation to be very rare.

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



More information about the webkit-unassigned mailing list