[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