Bug 135011 - CSS JIT: Clean up return path
Summary: CSS JIT: Clean up return path
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 134835
  Show dependency treegraph
 
Reported: 2014-07-16 23:32 PDT by Yusuke Suzuki
Modified: 2014-07-17 00:58 PDT (History)
1 user (show)

See Also:


Attachments
Patch (7.46 KB, patch)
2014-07-16 23:33 PDT, Yusuke Suzuki
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2014-07-16 23:32:42 PDT
Before adding early return path for PseudoElement, we need to simplify the existing return path in CSS JIT.
Comment 1 Yusuke Suzuki 2014-07-16 23:33:54 PDT
Created attachment 235054 [details]
Patch
Comment 2 Benjamin Poulain 2014-07-16 23:49:12 PDT
Comment on attachment 235054 [details]
Patch

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

This looks correct to me.

> Source/WebCore/cssjit/SelectorCompiler.cpp:1193
> +    StackAllocator::StackReference baselineStackReference;

I think "baseline" is a little too generic for the name. One could think it is the top of the stack for this function.

What about startOfDiscardableStack, temporaryStackBase, ... or something like that?

> Source/WebCore/cssjit/StackAllocator.h:156
> +    void popAndDiscardUpToBaseline()
> +    {
> +        if (!m_offsetFromTop)
> +            return;
> +        m_assembler.addPtr(JSC::MacroAssembler::TrustedImm32(m_offsetFromTop), JSC::MacroAssembler::stackPointerRegister);
> +        m_offsetFromTop = 0;
> +    }
> +

This is unused.
Comment 3 Yusuke Suzuki 2014-07-16 23:58:22 PDT
Comment on attachment 235054 [details]
Patch

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

Thank you for your comments. They are reasonable. I'll reflect your comments and land it :)

>> Source/WebCore/cssjit/SelectorCompiler.cpp:1193
>> +    StackAllocator::StackReference baselineStackReference;
> 
> I think "baseline" is a little too generic for the name. One could think it is the top of the stack for this function.
> 
> What about startOfDiscardableStack, temporaryStackBase, ... or something like that?

Right. `temporaryStackBase` looks suitable for me :) I'll change it.

>> Source/WebCore/cssjit/StackAllocator.h:156
>> +
> 
> This is unused.

Oops! Thank you, removing.
Comment 4 Yusuke Suzuki 2014-07-17 00:58:55 PDT
Committed r171177: <http://trac.webkit.org/changeset/171177>