Bug 151668

Created attachment 266260 [details] work in progress

Created attachment 266287 [details] the patch

Comment on attachment 266287 [details] the patch r=me if you make it apply cleanly.

Created attachment 266289 [details] patch for landing Rebased

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.

Hmmm I'll wait some more on those builds.

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?

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

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

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 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?

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