Bug 151668 - B3 stackmaps should support early clobber
Summary: B3 stackmaps should support early clobber
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 151667
  Show dependency treegraph
 
Reported: 2015-11-30 12:23 PST by Filip Pizlo
Modified: 2015-11-30 17:27 PST (History)
12 users (show)

See Also:


Attachments
work in progress (12.74 KB, patch)
2015-11-30 12:46 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (23.31 KB, patch)
2015-11-30 15:27 PST, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff
patch for landing (22.93 KB, patch)
2015-11-30 15:32 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-11-30 12:23:25 PST
Currently we almost support this with LateUse, but that's not really good enough.
Comment 1 Filip Pizlo 2015-11-30 12:46:28 PST
Created attachment 266260 [details]
work in progress
Comment 2 Filip Pizlo 2015-11-30 15:27:06 PST
Created attachment 266287 [details]
the patch
Comment 3 Geoffrey Garen 2015-11-30 15:30:35 PST
Comment on attachment 266287 [details]
the patch

r=me if you make it apply cleanly.
Comment 4 Filip Pizlo 2015-11-30 15:32:09 PST
Created attachment 266289 [details]
patch for landing

Rebased
Comment 5 WebKit Commit Bot 2015-11-30 15:33:39 PST
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.
Comment 6 Filip Pizlo 2015-11-30 15:40:54 PST
Hmmm I'll wait some more on those builds.
Comment 7 Benjamin Poulain 2015-11-30 15:43:06 PST
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?
Comment 8 Filip Pizlo 2015-11-30 15:48:31 PST
(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.
Comment 9 Filip Pizlo 2015-11-30 16:05:24 PST
Landed in http://trac.webkit.org/changeset/192841
Comment 10 Benjamin Poulain 2015-11-30 16:50:31 PST
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.
Comment 11 Filip Pizlo 2015-11-30 17:05:01 PST
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?
Comment 12 Benjamin Poulain 2015-11-30 17:27:30 PST
(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.