Bug 151668 – B3 stackmaps should support early clobber

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Currently we almost support this with LateUse, but that's not really good enough.

[Filip Pizlo]

Created attachment 266260 [details]
work in progress

[Filip Pizlo]

Created attachment 266287 [details]
the patch

[Geoffrey Garen]

Comment on attachment 266287 [details]
the patch

r=me if you make it apply cleanly.

[Filip Pizlo]

Created attachment 266289 [details]
patch for landing

Rebased

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:133:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:63:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3353:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3364:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3379:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:102:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:152:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:160:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:206:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:209:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:318:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 12 in 13 files


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

[Filip Pizlo]

Hmmm I'll wait some more on those builds.

[Benjamin Poulain]

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

> Source/JavaScriptCore/ChangeLog:3
> +        B3 stackmaps should support early clobber

What about saying "reserved registers" instead of early clobber?

It is a bit weird to have a clobber set *before* the instructions.

> Source/JavaScriptCore/b3/B3StackmapSpecial.h:51
>      const RegisterSet& extraClobberedRegs(Air::Inst&) override;
> +    const RegisterSet& extraEarlyClobberedRegs(Air::Inst&) override;

Move it before extraClobberedRegs?
Rename extraClobberedRegs to extraLateClobberedRegs?

[Filip Pizlo]

(In reply to comment #7)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266287&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        B3 stackmaps should support early clobber
> 
> What about saying "reserved registers" instead of early clobber?

I think that clobber has a more precise meaning.

> 
> It is a bit weird to have a clobber set *before* the instructions.

It's just like late use in that regard.

> 
> > Source/JavaScriptCore/b3/B3StackmapSpecial.h:51
> >      const RegisterSet& extraClobberedRegs(Air::Inst&) override;
> > +    const RegisterSet& extraEarlyClobberedRegs(Air::Inst&) override;
> 
> Move it before extraClobberedRegs?

Yeah, I'll do that.

> Rename extraClobberedRegs to extraLateClobberedRegs?

I ended up not doing this inside Air because there the extraClobberedRegs is the one that makes sense - it applies to its own instruction.

[Filip Pizlo]

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

[Benjamin Poulain]

Comment on attachment 266289 [details]
patch for landing

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

> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:208
> +                            addEdge(Tmp(reg), liveTmp);

This could add edges between FP and GP, preventing the algorithm from converging.

[Filip Pizlo]

Comment on attachment 266289 [details]
patch for landing

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

>> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:208
>> +                            addEdge(Tmp(reg), liveTmp);
> 
> This could add edges between FP and GP, preventing the algorithm from converging.

Ooops.  So it should be guarded by a check for liveTmp's type?

[Benjamin Poulain]

(In reply to comment #11)
> Comment on attachment 266289 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266289&action=review
> 
> >> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:208
> >> +                            addEdge(Tmp(reg), liveTmp);
> > 
> > This could add edges between FP and GP, preventing the algorithm from converging.
> 
> Ooops.  So it should be guarded by a check for liveTmp's type?

Yep. What is happening is we would have interference edges that never go away and we keep spilling (select spill).
It would eventually color but the result could be shit.

I'll have the new Liveness ready soon. That will fix the issue.