Bug 124094 – Switch FTL GetById/PutById IC's over to using AnyRegCC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 216475 [details]
just the beginning

[Filip Pizlo]

Created attachment 216483 [details]
work in progress

Starting to get there.  But still crashing in a bunch of tests.

[Filip Pizlo]

Created attachment 216495 [details]
crashes later than before

[Filip Pizlo]

Created attachment 216497 [details]
slowly making it work and stuff

More stuff still needs to be done

[Filip Pizlo]

Created attachment 216499 [details]
the patch

[Filip Pizlo]

Created attachment 216500 [details]
the patch

[Filip Pizlo]

Created attachment 216501 [details]
the patch

Just rebasing and refining the changelog.

[Sam Weinig]

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.

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 216502 [details]
the patch

Addressed Sam's comments and fixed some architectures.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/159039