[Webkit-unassigned] [Bug 143807] Extract the allocation profile from JSFunction into a rare object

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 250889
  --> https://bugs.webkit.org/attachment.cgi?id=250889
Fixed patch

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

LGTM other than maybe using one too many GPRs.  When you're ready to request review and have the reviewed patch committed, please set the review bit to "?" and the commit-queue bit to "?" as well.  Then whoever reviews this can set both bits to "+" which will trigger the automatic commit bot.

Our usual procedure for posting patches is:

- Create a bug and upload a patch as you have done.
- Post performance results as you have done, if applicable (some patches can safely be presumed to be perf-neutral).
- Mark the patch r?/cq?.
- Add appropriate reviewers to the CC on the bug.  That would be me, ggaren at apple.com, and msaboff at apple.com.  Maybe others.
- Send e-mail to the reviewers to poke them.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3490
> +        GPRTemporary rareData(this);

See below...

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3561
> +        GPRTemporary rareData(this);

Note that this increases the GPR usage of this code snippet, which then evicts other things from registers in case of pressure.  It might be nice to avoid using a separate GPR for this...

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3579
> -        
> -        m_jit.loadPtr(JITCompiler::Address(calleeGPR, JSFunction::offsetOfAllocationProfile() + ObjectAllocationProfile::offsetOfAllocator()), allocatorGPR);
> -        m_jit.loadPtr(JITCompiler::Address(calleeGPR, JSFunction::offsetOfAllocationProfile() + ObjectAllocationProfile::offsetOfStructure()), structureGPR);
> -        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, allocatorGPR));
> +
> +        m_jit.loadPtr(JITCompiler::Address(calleeGPR, JSFunction::offsetOfRareData()), rareDataGPR);
> +        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, rareDataGPR));
> +        m_jit.loadPtr(JITCompiler::Address(rareDataGPR, FunctionRareData::offsetOfAllocationProfile() + ObjectAllocationProfile::offsetOfAllocator()), allocatorGPR);
> +        m_jit.loadPtr(JITCompiler::Address(rareDataGPR, FunctionRareData::offsetOfAllocationProfile() + ObjectAllocationProfile::offsetOfStructure()), structureGPR);

Is it clear that you need a separate GPR for rareDataGPR?  Could you instead use structureGPR?  I believe that this code would work fine:

        m_jit.loadPtr(JITCompiler::Address(calleeGPR, JSFunction::offsetOfRareData()), structureGPR);
        slowPath.append(m_jit.branchTestPtr(MacroAssembler::Zero, structureGPR));
        m_jit.loadPtr(JITCompiler::Address(structureGPR, FunctionRareData::offsetOfAllocationProfile() + ObjectAllocationProfile::offsetOfAllocator()), allocatorGPR);
        m_jit.loadPtr(JITCompiler::Address(structureGPR, FunctionRareData::offsetOfAllocationProfile() + ObjectAllocationProfile::offsetOfStructure()), structureGPR);

This works because we only need the rareData until we load the structure, and it's OK to issue a load instruction that uses/defs the same register.  Such a sequence would be strictly better from a register pressure perspective.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150416/2e2a8a66/attachment.html>