Bug 151174

Created attachment 266332 [details] work in progress

Created attachment 266333 [details] the patch

Attachment 266333 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirUseCounts.h:80: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:89: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:90: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:91: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 266333 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=266333&action=review r=me > Source/JavaScriptCore/b3/B3UseCounts.h:45 > - > + Revert-o. > Source/JavaScriptCore/dfg/DFGCommon.h:41 > -#define FTL_USES_B3 0 > +#define FTL_USES_B3 1 Revert-o. I like the enthusiasm, though :P. > Source/JavaScriptCore/runtime/Options.h:341 > + v(double, rareBlockPenalty, 0.001, nullptr) \ Do you know of a reason this penalty should not be infinite, or is this just a guesstimate?

View in context: https://bugs.webkit.org/attachment.cgi?id=266333&action=review > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:89 > + : m_code(code) I would really prefer not to keep a reference to the code. That way it is easy to constrain it to build() and build() only. > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:564 > + // Higher score means more desirable to spill. Lower scores maximize the likelihood that a tmp There is a FIXME above about implementing something less stupid. Can you please remove it? > Source/JavaScriptCore/dfg/DFGCommon.h:41 > -#define FTL_USES_B3 0 > +#define FTL_USES_B3 1 Don't forget this!

(In reply to comment #4) > Comment on attachment 266333 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266333&action=review > > r=me > > > Source/JavaScriptCore/b3/B3UseCounts.h:45 > > - > > + > > Revert-o. > > > Source/JavaScriptCore/dfg/DFGCommon.h:41 > > -#define FTL_USES_B3 0 > > +#define FTL_USES_B3 1 > > Revert-o. I like the enthusiasm, though :P. > > > Source/JavaScriptCore/runtime/Options.h:341 > > + v(double, rareBlockPenalty, 0.001, nullptr) \ > > Do you know of a reason this penalty should not be infinite, or is this just > a guesstimate? Yeah, I do! And by "infinite" I assume you mean "0". If we really used +Inf then it would infinitely reward uses on rare blocks, which is clearly not right. If it was 0 then the score of all variables that only got used on slow paths would all be +Inf, since the use counts would be 0. That means that any time we encountered register pressure, we would spill some variable in some slow path at random. That's totally bad. For starters, we have no information that spilling that variable would relieve any register pressure. But also, that variable could be one of those that can always get a register no matter what. Using a rareBlockPenalty that is small but not zero ensures that when we try to find a variable with the highest score to spill, we won't be picking at random. Being a variable that only gets used on slow paths isn't enough to automatically get spilled - we will still pick among those variables according to other factors, like the interference degree.

(In reply to comment #5) > View in context: > https://bugs.webkit.org/attachment.cgi?id=266333&action=review > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:89 > > + : m_code(code) > > I would really prefer not to keep a reference to the code. > That way it is easy to constrain it to build() and build() only. What is the benefit of constraining it to build()? In all of the other phases, we sort of assume that Code& is always available. I don't view it as off-limits to have useful meta-data on Code, so it seems fair game to make Code& available in more places. > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:564 > > + // Higher score means more desirable to spill. Lower scores maximize the likelihood that a tmp > > There is a FIXME above about implementing something less stupid. Can you > please remove it? Will do!

Created attachment 266335 [details] patch for landing

Attachment 266335 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirUseCounts.h:80: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:89: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:90: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:91: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 266335 [details] patch for landing Actually, I'll commit myself.

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