Summary: | B3 peephole optimizations should live in their own file, and be used by FTLOutput.cpp | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robin Morisset <rmorisset> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Robin Morisset <rmorisset> | ||||||||||||
Status: | NEW --- | ||||||||||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Robin Morisset
2019-04-03 16:44:30 PDT
Created attachment 366733 [details]
Patch
Most of this big patch is a fairly mechanical refactoring, taking the peephole optimizations out of B3ReduceStrength.
I also then used these peephole optimizations in FTLOutput and a bit in B3LowerMacros, to provide motivation for the refactoring.
Attachment 366733 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:203: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:291: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:352: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:358: Missing space before { [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:380: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:499: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:693: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:976: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:1100: Missing space before ( in switch( [whitespace/parens] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:1279: Missing space before ( in switch( [whitespace/parens] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:1562: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.cpp:1562: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.h:75: The parameter name "origin" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.h:107: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.h:107: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3PeepholeOptimizer.h:138: The parameter name "arguments" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 16 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 366765 [details]
Patch
Fix style issues + a build issue caused by a missing import.
Created attachment 366774 [details]
Patch
Fix a missing "appendToCurrentBlock" in reduceLoadStore, that was causing a validation failure due to some orphaned constants.
Some basic numbers, from running JetStream2 with --reportFTLCompileTimes=1 and --useConcurrentJIT=0 (to minimize noise), and averaging the times (all in ms): Baseline: 4.17321 4.16281 4.0801 4.06007 After patch: 4.11306 4.08184 4.10042 4.07414 So nothing particularly impressive, but there is no regression, and there might be a small progression. Since the main motivation for this patch was the refactoring, independent of the possible perf improvement of doing reducing on the fly in FTLOutput.h, I still think it is worth it. Created attachment 367692 [details]
Patch
Just rebased so it applies cleanly on ToT.
Comment on attachment 367692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367692&action=review On the one hand, I like this change. But on the other hand, it doesn't seem like it's a measurable improvement and it means that adding new strength-reduction rules may be annoying since for some opcodes you need to have a case in reduceStrength and a function on the PeepholeOptimizer. I think that we should try to have the simplest compiler we can get away with while getting great perf. So, that means not taking this change. On the other hand, if using this change in conjunction with some other change does result in a measurable improvement, then we should take it. So, that means not taking the change until there is another change that makes this one valuable. > Source/JavaScriptCore/b3/B3LowerMacros.cpp:158 > + Value* divResult = peephole.reduceDiv(m_value->type(), m_origin, m_value->child(0), m_value->child(1), true); WebKit style says you should never pass the literal true or false as an argument to a function except if the name of the function makes it obvious what the meaning of that boolean is. There are two accepted ways of side-stepping this issue: 1) Always name the argument at the callsite: bool shouldPropagateType = true; thingy->fooBar(stuff, shouldPropagateType); 2) Replace with an enum: enum ShouldPropagateTypeMode { ShouldPropagateType, ShouldNotPropagateType }; thingy->fooBar(stuff, ShouldPropagateType); The second one is preferred. Created attachment 368204 [details]
Patch
Thanks for the review.
The only change is adding enums for the two places where I was passing booleans (IsChillMode and MayGiveUpMode).
Following your comment, I will not ask for another review or try to land this until I write the AI phase that will use it (and which was the motivation for this patch in the first place).
|