Bug 152315 - Introducing ScratchRegisterAllocator::PreservedState.
Summary: Introducing ScratchRegisterAllocator::PreservedState.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-15 14:34 PST by Mark Lam
Modified: 2015-12-16 08:26 PST (History)
5 users (show)

See Also:


Attachments
proposed patch. (19.28 KB, patch)
2015-12-15 14:42 PST, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
Patch with build fix. (20.91 KB, patch)
2015-12-15 15:51 PST, Mark Lam
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-12-15 14:34:09 PST
restoreReusedRegistersByPopping() should always be called with 2 values that matches the expectation of preserveReusedRegistersByPushing().  Those 2 values are the number of bytes preserved and the ExtraStackSpace requirement.  By encapsulating them in a ScratchRegisterAllocator::PreservedState, we can make this less error prone by only passing restoreReusedRegistersByPopping() the appropriate PreservedState that its matching preserveReusedRegistersByPushing() returned.
Comment 1 Mark Lam 2015-12-15 14:42:39 PST
Created attachment 267394 [details]
proposed patch.
Comment 2 Geoffrey Garen 2015-12-15 14:58:27 PST
Patch looks good.

I think we usually say "Saved", which is slightly shorter than "Preserved."

EWS is angry, so r-:

/Volumes/Data/EWS/WebKit/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/PolymorphicAccess.h:36:10: fatal error: 'ScratchRegisterAllocator.h' file not found
#include "ScratchRegisterAllocator.h"
Comment 3 Mark Lam 2015-12-15 15:24:31 PST
(In reply to comment #2)
> I think we usually say "Saved", which is slightly shorter than "Preserved."

Talked with Geoff offline.  The function that generates thus artifact is named preserveReusedRegistersByPushing().  It would be a mismatch to name the artifact SavedState while keeping the function as is.  There are also several other preserve...() functions in ScratchRegisterAllocator unrelated to the patch here.

In light of that, for now, we'll stick with calling it PreservedState.

> EWS is angry, so r-:

Will fix.
Comment 4 Mark Lam 2015-12-15 15:51:07 PST
Created attachment 267404 [details]
Patch with build fix.
Comment 5 Geoffrey Garen 2015-12-15 17:01:29 PST
Comment on attachment 267404 [details]
Patch with build fix.

r=me

EWS looks angry for unrelated reasons, so you'll need to land manually.
Comment 6 Mark Lam 2015-12-15 17:06:31 PST
Thanks for the review.  Landed in r194126: <http://trac.webkit.org/r194126>.
Comment 7 Saam Barati 2015-12-16 08:26:23 PST
Comment on attachment 267404 [details]
Patch with build fix.

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

> Source/JavaScriptCore/jit/ScratchRegisterAllocator.h:74
> +        PreservedState()
> +            : PreservedState(0)
> +        { }

Since we have a default empty constructor I think it's worth having an operator bool indicating that a PreservedState is valid or not.
We could just make the invalid state have UINT_MAX for preserved bytes.
We can assert inside restore / preserve reusedRegisters that we have a valid state.
Comment 8 Saam Barati 2015-12-16 08:26:51 PST
(In reply to comment #7)
> Comment on attachment 267404 [details]
> Patch with build fix.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267404&action=review
> 
> > Source/JavaScriptCore/jit/ScratchRegisterAllocator.h:74
> > +        PreservedState()
> > +            : PreservedState(0)
> > +        { }
> 
> Since we have a default empty constructor I think it's worth having an
> operator bool indicating that a PreservedState is valid or not.
> We could just make the invalid state have UINT_MAX for preserved bytes.
> We can assert inside restore / preserve reusedRegisters that we have a valid
> state.

I'll write a patch.