Bug 160010 – FTL snippet generators should be able to request a different register for output and input

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 284186 [details]
the patch

[Mark Lam]

Comment on attachment 284186 [details]
the patch

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

> Source/JavaScriptCore/b3/B3ValueRep.h:69
> +        

nit: please remove empty spaces here.

> Source/JavaScriptCore/b3/B3ValueRep.h:78
> -
> +        

Unnecessary change.  Please remove.

[Saam Barati]

Comment on attachment 284186 [details]
the patch

r=me
Can you add some testb3 specific tests that uses a patchpoint and SomeEarlyRegister to ensure that no input has the same register as the output register.

[Mark Lam]

Comment on attachment 284186 [details]
the patch

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

> Source/JavaScriptCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=160010

Can you also add the radar url here after the bugzilla one?  I think it's customary to do that now.

[Filip Pizlo]

Thanks for the feedback guys!

I've got a 100% repro test that I'll also add:

var toggle = 0;

function bar()
{
    if (toggle ^= 1)
        return 42;
    else
        return {valueOf: function() { return 42; }};
}

noInline(bar);

function baz()
{
    return 7;
}

noInline(baz);

function foo()
{
    return bar() ^ baz();
}

noInline(foo);

for (var i = 0; i < 100000; ++i) {
    var result = foo();
    if (result != 45)
        throw "Error: bad result: " + result;
}

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 284186 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284186&action=review
> 
> > Source/JavaScriptCore/b3/B3ValueRep.h:69
> > +        
> 
> nit: please remove empty spaces here.

Fixed.

> 
> > Source/JavaScriptCore/b3/B3ValueRep.h:78
> > -
> > +        
> 
> Unnecessary change.  Please remove.

Fixed.

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 284186 [details]
> the patch
> 
> r=me
> Can you add some testb3 specific tests that uses a patchpoint and
> SomeEarlyRegister to ensure that no input has the same register as the
> output register.

Sure!  I added a test that runs the same compile in two modes: one in which an arrangement of patchpoints causes some patchpoint to see the same register for input and output because it's presented as an ultra-tempting register coalescing oppportunity, and another in which we use SomeEarlyRegister to prevent coalescing.

[Filip Pizlo]

(In reply to comment #4)
> Comment on attachment 284186 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284186&action=review
> 
> > Source/JavaScriptCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=160010
> 
> Can you also add the radar url here after the bugzilla one?  I think it's
> customary to do that now.

OK!

[Filip Pizlo]

Created attachment 284191 [details]
patch for landing

[WebKit Commit Bot]

Attachment 284191 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:12382:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:12391:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:12404:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/203488