Bug 163622 – SpeculativeJIT::compileTryGetById needs to pass in NeedsToSpill along both the cell speculation and untyped speculation path

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

[Saam Barati]

...

[Saam Barati]

Created attachment 292268 [details]
patch

[JF Bastien]

Comment on attachment 292268 [details]
patch

Looks good!

[Yusuke Suzuki]

Comment on attachment 292268 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        that it allocated for it after the TryGetById node executed.

Yeah, if base's use count remains, we still should keep this in that register!

[Geoffrey Garen]

Comment on attachment 292268 [details]
patch

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

> JSTests/stress/try-get-by-id-should-spill-registers-dfg.js:10
> +let f = createBuiltin(`(function (arg) { 
> +        let r = @tryGetById(arg, "prototype");
> +        if (arg !== true) throw new @Error("Bad clobber of arg");
> +        return r;
> +    })`);
> +noInline(f);
> +
> +for (let i = 0; i < 10000; i++) {
> +    f(true);
> +}

Looks like you used tabs instead of spaces?

[Saam Barati]

Comment on attachment 292268 [details]
patch

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

>> JSTests/stress/try-get-by-id-should-spill-registers-dfg.js:10
>> +}
> 
> Looks like you used tabs instead of spaces?

I don't think I did but my indentation is a bit off. I'll fix before landing.

[Saam Barati]

Created attachment 292403 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 292403 [details]
patch for landing

Clearing flags on attachment: 292403

Committed r207697: <http://trac.webkit.org/changeset/207697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.