[webkit-reviews] review granted: [Bug 133506] CSS JIT: Clean up StackAllocator : [Attachment 232469] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 133506: CSS JIT: Clean up StackAllocator
https://bugs.webkit.org/show_bug.cgi?id=133506

Attachment 232469: Patch
https://bugs.webkit.org/attachment.cgi?id=232469&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.