Bug 196578 – B3 peephole optimizations should live in their own file, and be used by FTLOutput.cpp

• Component: JavaScriptCore
• Status: NEW

[Robin Morisset]

Currently, the peephole optimizations are part of B3ReduceStrength.
I plan to make it possible to run them on the fly as we create new values and put them in their own file (form which they will be publicly visible) so that FTLOutput.h can call them directly.
This should help feed B3 some leaner/faster to compile code.

In the long term, this refactoring should also help create an abstract interpreter that also calls these peephole optimizations, feeding them extra information.

[Robin Morisset]

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.

[EWS Watchlist]

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.

[Robin Morisset]

Created attachment 366765 [details]
Patch

Fix style issues + a build issue caused by a missing import.

[Robin Morisset]

Created attachment 366774 [details]
Patch

Fix a missing "appendToCurrentBlock" in reduceLoadStore, that was causing a validation failure due to some orphaned constants.

[Robin Morisset]

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.

[Robin Morisset]

Created attachment 367692 [details]
Patch

Just rebased so it applies cleanly on ToT.

[Filip Pizlo]

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.

[Robin Morisset]

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