Bug 133506 - CSS JIT: Clean up StackAllocator
Summary: CSS JIT: Clean up StackAllocator
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:
 
Reported: 2014-06-04 02:03 PDT by Yusuke Suzuki
Modified: 2014-06-05 11:40 PDT (History)
2 users (show)

See Also:

Attachments
Patch (8.36 KB, patch)
2014-06-04 02:06 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (8.53 KB, patch)
2014-06-05 08:34 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2014-06-04 02:03:47 PDT 
Cleaning up CSS JIT SatkcAcllocator code.
Comment 1 Yusuke Suzuki 2014-06-04 02:06:19 PDT 
Created attachment 232469 [details]
Patch
Comment 2 Benjamin Poulain 2014-06-04 16:51:23 PDT 
Comment on attachment 232469 [details]
Patch

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

Looks good to me.

> Source/WebCore/cssjit/StackAllocator.h:85
>          if (registerCount % 2) {
> -            m_assembler.pushToSave(registerIDs[registerCount - 1]);
> -            m_offsetFromTop += stackUnitInBytes;
> -            stackReferences.append(StackReference(m_offsetFromTop));
> +            stackReferences.append(push(registerIDs[registerCount - 1]));

WebKit coding style: the brackets should go away when the block only has one line.

> Source/WebCore/cssjit/StackAllocator.h:90
>          for (auto registerID : registerIDs) {
> -            m_assembler.pushToSave(registerID);
> -            m_offsetFromTop += stackUnitInBytes;
> -            stackReferences.append(StackReference(m_offsetFromTop));
> +            stackReferences.append(push(registerID));
>          }

Ditto.

> Source/WebCore/cssjit/StackAllocator.h:114
>          if (registerCountOdd) {
> -            RELEASE_ASSERT(stackReferences[registerCount - 1] == m_offsetFromTop);
> -            RELEASE_ASSERT(m_offsetFromTop >= stackUnitInBytes);
> -            m_offsetFromTop -= stackUnitInBytes;
> -            m_assembler.popToRestore(registerIDs[registerCount - 1]);
> +            pop(stackReferences[registerCount - 1], registerIDs[registerCount - 1]);
>          }

Ditto.

> Source/WebCore/cssjit/StackAllocator.h:126
>          for (unsigned i = registerCount; i > 0; --i) {
> -            RELEASE_ASSERT(stackReferences[i - 1] == m_offsetFromTop);
> -            RELEASE_ASSERT(m_offsetFromTop >= stackUnitInBytes);
> -            m_offsetFromTop -= stackUnitInBytes;
> -            m_assembler.popToRestore(registerIDs[i - 1]);
> +            pop(stackReferences[i - 1], registerIDs[i - 1]);
>          }

Ditto.
Comment 3 Yusuke Suzuki 2014-06-05 08:34:38 PDT 
Created attachment 232551 [details]
Patch
Comment 4 Yusuke Suzuki 2014-06-05 08:34:58 PDT 
Comment on attachment 232469 [details]
Patch

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

Thank you for your review.

>> Source/WebCore/cssjit/StackAllocator.h:85
>> +            stackReferences.append(push(registerIDs[registerCount - 1]));
> 
> WebKit coding style: the brackets should go away when the block only has one line.

Oh! I've missed it. Thank you.
Comment 5 Benjamin Poulain 2014-06-05 11:10:36 PDT 
Comment on attachment 232551 [details]
Patch

Looks good to me.
Comment 6 WebKit Commit Bot 2014-06-05 11:40:54 PDT 
Comment on attachment 232551 [details]
Patch

Clearing flags on attachment: 232551

Committed r169621: <http://trac.webkit.org/changeset/169621>
Comment 7 WebKit Commit Bot 2014-06-05 11:40:56 PDT 
All reviewed patches have been landed.  Closing bug.