Bug 124094 - Switch FTL GetById/PutById IC's over to using AnyRegCC
Summary: Switch FTL GetById/PutById IC's over to using AnyRegCC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 112840
  Show dependency treegraph
 
Reported: 2013-11-08 18:42 PST by Filip Pizlo
Modified: 2013-11-10 23:29 PST (History)
12 users (show)

See Also:


Attachments
just the beginning (4.76 KB, patch)
2013-11-08 18:43 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
work in progress (18.74 KB, patch)
2013-11-08 22:44 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
crashes later than before (23.31 KB, patch)
2013-11-09 13:09 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
slowly making it work and stuff (28.52 KB, patch)
2013-11-09 13:56 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (39.27 KB, patch)
2013-11-09 16:44 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (41.47 KB, patch)
2013-11-09 16:57 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (41.43 KB, patch)
2013-11-09 17:03 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (42.09 KB, patch)
2013-11-09 17:38 PST, Filip Pizlo
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-11-08 18:42:29 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2013-11-08 18:43:23 PST
Created attachment 216475 [details]
just the beginning
Comment 2 Filip Pizlo 2013-11-08 22:44:33 PST
Created attachment 216483 [details]
work in progress

Starting to get there.  But still crashing in a bunch of tests.
Comment 3 Filip Pizlo 2013-11-09 13:09:58 PST
Created attachment 216495 [details]
crashes later than before
Comment 4 Filip Pizlo 2013-11-09 13:56:59 PST
Created attachment 216497 [details]
slowly making it work and stuff

More stuff still needs to be done
Comment 5 Filip Pizlo 2013-11-09 16:44:43 PST
Created attachment 216499 [details]
the patch
Comment 6 Filip Pizlo 2013-11-09 16:57:01 PST
Created attachment 216500 [details]
the patch
Comment 7 Filip Pizlo 2013-11-09 17:03:20 PST
Created attachment 216501 [details]
the patch

Just rebasing and refining the changelog.
Comment 8 Sam Weinig 2013-11-09 17:29:21 PST
Comment on attachment 216501 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        JIT (like the DFG) will not pin IC operands to any registers a prior but will allow

"a prior" -> "a priori".

> Source/JavaScriptCore/ChangeLog:47
> +          and eclectic SIB encodings. I changed that to have magic constants, for now.

SIB? What is the long term plan to replace the magic constants?

> Source/JavaScriptCore/ChangeLog:57
> +        - We assumed that r10 is callee-saved. It's not. That one dude's PPT about x86-64
> +          cdecl that I found on the intertubes was not a trustworthy source of information,
> +          apparently.

LOL.

> Source/JavaScriptCore/ftl/FTLInlineCacheSize.cpp:38
> +    return 29;

This (and its friends) deserve a comment.

> Source/JavaScriptCore/ftl/FTLStackMaps.cpp:71
> +    return FTL::Location::forStackmaps(0, *this).directGPR();

You should use nullptr rather than 0.
Comment 9 Filip Pizlo 2013-11-09 17:32:03 PST
(In reply to comment #8)
> (From update of attachment 216501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216501&action=review
> 
> > Source/JavaScriptCore/ChangeLog:18
> > +        JIT (like the DFG) will not pin IC operands to any registers a prior but will allow
> 
> "a prior" -> "a priori".
> 
> > Source/JavaScriptCore/ChangeLog:47
> > +          and eclectic SIB encodings. I changed that to have magic constants, for now.
> 
> SIB?

Scaled index byte.

It's an intel thing.

> What is the long term plan to replace the magic constants?

I haven't decided yet, if I think that it's bad to have these constants. Ideally you'd loop over all possible combinations of registers and such, and find the max size.  Or you'd find the size for some representative combo of registers, and force out-lining (i.e. jump to a stub) in cases where you used the bad register combinations.  I'm not convinced that either of these is better than the magical constants.

> 
> > Source/JavaScriptCore/ChangeLog:57
> > +        - We assumed that r10 is callee-saved. It's not. That one dude's PPT about x86-64
> > +          cdecl that I found on the intertubes was not a trustworthy source of information,
> > +          apparently.
> 
> LOL.
> 
> > Source/JavaScriptCore/ftl/FTLInlineCacheSize.cpp:38
> > +    return 29;
> 
> This (and its friends) deserve a comment.

OK.  But the comment will be: I tried random numbers until tests passed.

Fortunately we really do have enough test coverage at this point that this isn't a totally embarrassing thing to do.

> 
> > Source/JavaScriptCore/ftl/FTLStackMaps.cpp:71
> > +    return FTL::Location::forStackmaps(0, *this).directGPR();
> 
> You should use nullptr rather than 0.
Comment 10 Filip Pizlo 2013-11-09 17:38:32 PST
Created attachment 216502 [details]
the patch

Addressed Sam's comments and fixed some architectures.
Comment 11 Filip Pizlo 2013-11-10 23:29:07 PST
Landed in http://trac.webkit.org/changeset/159039