RESOLVED FIXED 153200
B3 should have basic path specialization
https://bugs.webkit.org/show_bug.cgi?id=153200
Summary B3 should have basic path specialization
Filip Pizlo
Reported 2016-01-17 21:38:54 PST
Patch forthcoming.
Attachments
it's a start (25.46 KB, patch)
2016-01-17 21:41 PST, Filip Pizlo
no flags
it is written (27.87 KB, patch)
2016-01-18 10:27 PST, Filip Pizlo
no flags
it specialized a check (42.69 KB, patch)
2016-01-18 11:52 PST, Filip Pizlo
no flags
starting to run real tests (46.23 KB, patch)
2016-01-18 15:12 PST, Filip Pizlo
no flags
more (70.09 KB, patch)
2016-01-19 14:56 PST, Filip Pizlo
no flags
even more (104.79 KB, patch)
2016-01-19 16:17 PST, Filip Pizlo
no flags
it made deltablue faster in FTL B3 than FTL LLVM (110.05 KB, patch)
2016-01-19 17:53 PST, Filip Pizlo
no flags
and now with a changelog (136.10 KB, patch)
2016-01-19 20:18 PST, Filip Pizlo
benjamin: review+
buildbot: commit-queue-
latest standings against LLVM (80.93 KB, text/plain)
2016-01-19 21:08 PST, Filip Pizlo
no flags
Archive of layout-test-results from ews116 for mac-yosemite (1.24 MB, application/zip)
2016-01-19 21:48 PST, Build Bot
no flags
patch for landing (143.26 KB, patch)
2016-01-20 18:44 PST, Filip Pizlo
no flags
Filip Pizlo
Comment 1 2016-01-17 21:41:38 PST
Created attachment 269210 [details] it's a start
Filip Pizlo
Comment 2 2016-01-18 10:27:37 PST
Created attachment 269229 [details] it is written
Filip Pizlo
Comment 3 2016-01-18 11:52:24 PST
Created attachment 269232 [details] it specialized a check
Filip Pizlo
Comment 4 2016-01-18 15:12:13 PST
Created attachment 269239 [details] starting to run real tests
Filip Pizlo
Comment 5 2016-01-18 18:46:37 PST
Wow, this is actually a regression! The conditional move is profitable even though it looks super ugly.
Filip Pizlo
Comment 6 2016-01-19 14:36:34 PST
I'm going to try this again.
Filip Pizlo
Comment 7 2016-01-19 14:56:21 PST
Filip Pizlo
Comment 8 2016-01-19 16:17:44 PST
Created attachment 269306 [details] even more Making it even more of a rampage by implementing SSA demotion and SSA fixup. 'Cause, you know, you can't really do taildup without those things.
Filip Pizlo
Comment 9 2016-01-19 17:53:00 PST
Created attachment 269317 [details] it made deltablue faster in FTL B3 than FTL LLVM
Filip Pizlo
Comment 10 2016-01-19 20:18:42 PST
Created attachment 269328 [details] and now with a changelog Still testing this.
Filip Pizlo
Comment 11 2016-01-19 20:31:13 PST
Wow, this is kind of a big deal. Benchmark report for Octane on shakezilla (MacBookPro11,3). VMs tested: "B3ToT" at /Volumes/Data/quartary/OpenSource/WebKitBuild/Release/jsc (r195308) "NewB3" at /Volumes/Data/quinary/OpenSource/WebKitBuild/Release/jsc (r195308) Collected 6 samples per benchmark/VM, with 6 VM invocations per benchmark. Emitted a call to gc() between sample measurements. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. B3ToT NewB3 encrypt 0.16682+-0.00770 0.16669+-0.00421 decrypt 2.87488+-0.06210 2.80440+-0.04123 might be 1.0251x faster deltablue x2 0.15536+-0.00832 ^ 0.13717+-0.00227 ^ definitely 1.1326x faster earley 0.27949+-0.00372 ? 0.28423+-0.00505 ? might be 1.0169x slower boyer 4.64508+-0.13043 4.63469+-0.17908 navier-stokes x2 4.81913+-0.02791 4.81354+-0.02552 raytrace x2 0.88046+-0.00661 ^ 0.86766+-0.00188 ^ definitely 1.0148x faster richards x2 0.09873+-0.00083 ^ 0.08979+-0.00096 ^ definitely 1.0996x faster splay x2 0.35560+-0.00450 0.35486+-0.00485 regexp x2 24.51355+-0.42627 ? 24.77818+-0.61266 ? might be 1.0108x slower pdfjs x2 37.48547+-0.14231 ? 37.60167+-0.15585 ? mandreel x2 48.95878+-0.33221 ^ 47.71011+-0.76387 ^ definitely 1.0262x faster gbemu x2 32.26218+-2.17414 31.54580+-0.23149 might be 1.0227x faster closure 0.57923+-0.00442 ? 0.58585+-0.00514 ? might be 1.0114x slower jquery 7.39526+-0.09223 7.36854+-0.03890 box2d x2 9.09089+-0.06643 9.07152+-0.07851 zlib x2 392.30406+-7.29783 ? 399.72401+-7.35874 ? might be 1.0189x slower typescript x2 667.76811+-18.52387 665.96224+-11.20068 <geometric> 5.50762+-0.03977 ^ 5.41406+-0.03001 ^ definitely 1.0173x faster
Filip Pizlo
Comment 12 2016-01-19 20:31:45 PST
Comment on attachment 269328 [details] and now with a changelog It's ready for review.
WebKit Commit Bot
Comment 13 2016-01-19 20:33:55 PST
Attachment 269328 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3Value.h:219: The parameter name "functor" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9091: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9105: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9106: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9106: 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/testb3.cpp:9107: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9134: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9162: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9176: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9177: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9178: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9178: 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/testb3.cpp:9179: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9180: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/B3ValueKey.h:88: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1598: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3FixSSA.cpp:141: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3FixSSA.cpp:146: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 18 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.
Filip Pizlo
Comment 14 2016-01-19 21:08:43 PST
Created attachment 269332 [details] latest standings against LLVM
Build Bot
Comment 15 2016-01-19 21:48:43 PST
Comment on attachment 269328 [details] and now with a changelog Attachment 269328 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/715396 New failing tests: js/regress/fold-multi-put-by-offset-to-replace-or-transition-put-by-offset.html js/regress/fold-multi-put-by-offset-to-put-by-offset.html js/regress/polymorphic-put-by-val-with-string.html js/regress/polymorphic-put-by-val-with-symbol.html js/regress/polymorphic-put-by-id.html js/regress/fold-multi-put-by-offset-to-poly-put-by-offset.html
Build Bot
Comment 16 2016-01-19 21:48:48 PST
Created attachment 269337 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Csaba Osztrogonác
Comment 17 2016-01-19 23:53:49 PST
Comment on attachment 269328 [details] and now with a changelog View in context: https://bugs.webkit.org/attachment.cgi?id=269328&action=review > Source/JavaScriptCore/dfg/DFGCommon.h:41 > +#define FTL_USES_B3 1 Please remove it before landing.
Geoffrey Garen
Comment 18 2016-01-20 11:31:55 PST
Regressions: Unexpected crashes (8) imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html [ Crash ] js/regress/fold-multi-get-by-offset-to-get-by-offset.html [ Crash ] js/regress/fold-multi-put-by-offset-to-poly-put-by-offset.html [ Crash ] js/regress/fold-multi-put-by-offset-to-put-by-offset.html [ Crash ] js/regress/fold-multi-put-by-offset-to-replace-or-transition-put-by-offset.html [ Crash ] js/regress/polymorphic-put-by-id.html [ Crash ] js/regress/polymorphic-put-by-val-with-string.html [ Crash ] js/regress/polymorphic-put-by-val-with-symbol.html [ Crash ]
Geoffrey Garen
Comment 19 2016-01-20 11:36:33 PST
Comment on attachment 269328 [details] and now with a changelog View in context: https://bugs.webkit.org/attachment.cgi?id=269328&action=review > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:72 > + // and they much not have too many successors. must
Benjamin Poulain
Comment 20 2016-01-20 17:18:00 PST
Comment on attachment 269328 [details] and now with a changelog View in context: https://bugs.webkit.org/attachment.cgi?id=269328&action=review > Source/JavaScriptCore/b3/B3ArgumentRegValue.h:47 > + Value* cloneImpl() const override; Shouldn't all the cloneImpl() be private? > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:105 > + ControlValue* jump = block->last()->as<ControlValue>(); > + if (jump->opcode() != Jump) It would be more generic to test block->numSuccessors() == 1. We don't really care about the kind of ControlValue for changing the flow graph. > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:146 > + fixSSA(m_proc); If I am not mistaken: This is only called if (changed == true). If all the blocks in "candidates" are reached by a branch instead of a jump, their values would be demoted but never put back in SSA. > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:202 > + void foldPathConstants() Clever. Shouldn't it have its own phase? It seems unrelated to tail duplication. > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:221 > + if (override.block->predecessor(0) != from) That's odd. I would think this does not happen since the previous phase does resetReachability(). > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:235 > + if (value->index() >= 150) > + return; > + if (value->index() < 50) > + return; You are losing me there. Why does this depends on the index? > Source/JavaScriptCore/b3/B3FixSSA.cpp:248 > + mapping.clear(); Why not declare mapping in the loop? > Source/JavaScriptCore/b3/air/AirCode.h:317 > + void forAllTmps(Arg::Type type, const Callback& callback) const This does not seem used anywhere. > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:104 > + if (m_unspillableTmps.contains(toSpill)) Are you sure that's not a regression? Adding to the m_spillWorklist is not uncommon. I fixed the same bug today, somewhat differently.
Benjamin Poulain
Comment 21 2016-01-20 17:26:29 PST
(In reply to comment #20) > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:104 > > + if (m_unspillableTmps.contains(toSpill)) > > Are you sure that's not a regression? Adding to the m_spillWorklist is not > uncommon. > > I fixed the same bug today, somewhat differently. That's probably okay actually. I think combine() is unlikely to modify the spill worklist since we use conservative heuristics.
Filip Pizlo
Comment 22 2016-01-20 17:31:45 PST
(In reply to comment #20) > Comment on attachment 269328 [details] > and now with a changelog > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269328&action=review > > > Source/JavaScriptCore/b3/B3ArgumentRegValue.h:47 > > + Value* cloneImpl() const override; > > Shouldn't all the cloneImpl() be private? Maybe? So far all of the methods in Value that are primarily for overriding in subclasses are protected. > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:105 > > + ControlValue* jump = block->last()->as<ControlValue>(); > > + if (jump->opcode() != Jump) > > It would be more generic to test block->numSuccessors() == 1. > We don't really care about the kind of ControlValue for changing the flow > graph. Currently it doesn't matter - the only other way to get one successor is to have a Switch with no cases and just a fall-through, but the solution there is to make strength-reduction fix those. As for future opcodes, testing for one successor is more generic but maybe less safe. Probably if we added a new opcode that had a single successor it would only be because that opcode did something special, which would make it illegal to do what this phase does. I mean, I guess we could test if numSuccessors is 1 and effects() is empty, but that feels like a bit much. > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:146 > > + fixSSA(m_proc); > > If I am not mistaken: > > This is only called if (changed == true). If all the blocks in "candidates" > are reached by a branch instead of a jump, their values would be demoted but > never put back in SSA. D'oh! Good catch. Obviously, after we call demoteValues() we need to commit to calling fixSSA(). > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:202 > > + void foldPathConstants() > > Clever. > > Shouldn't it have its own phase? It seems unrelated to tail duplication. That's a good idea. I'll do that. > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:221 > > + if (override.block->predecessor(0) != from) > > That's odd. I would think this does not happen since the previous phase does > resetReachability(). Yeah, this should be an assertion. > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:235 > > + if (value->index() >= 150) > > + return; > > + if (value->index() < 50) > > + return; > > You are losing me there. Why does this depends on the index? Ha! This was debug code. This machinery was failing some test. This would explain why my deltablue speed-up went down from 20% to 13%. ;-) I'll retest everything after removing this garbage. > > > Source/JavaScriptCore/b3/B3FixSSA.cpp:248 > > + mapping.clear(); > > Why not declare mapping in the loop? Because I'm fantasizing about a future utopia where HashMap::clear() doesn't always free the backing store. > > > Source/JavaScriptCore/b3/air/AirCode.h:317 > > + void forAllTmps(Arg::Type type, const Callback& callback) const > > This does not seem used anywhere. I'll kill it. > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:104 > > + if (m_unspillableTmps.contains(toSpill)) > > Are you sure that's not a regression? Adding to the m_spillWorklist is not > uncommon. > > I fixed the same bug today, somewhat differently. I'll revert my changes to IRC so long as doing so doesn't break anything. This change did speed up testComplex with taildup.
Filip Pizlo
Comment 23 2016-01-20 17:40:26 PST
(In reply to comment #22) > (In reply to comment #20) > > Comment on attachment 269328 [details] > > and now with a changelog > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=269328&action=review > > > > > Source/JavaScriptCore/b3/B3ArgumentRegValue.h:47 > > > + Value* cloneImpl() const override; > > > > Shouldn't all the cloneImpl() be private? > > Maybe? So far all of the methods in Value that are primarily for overriding > in subclasses are protected. > > > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:105 > > > + ControlValue* jump = block->last()->as<ControlValue>(); > > > + if (jump->opcode() != Jump) > > > > It would be more generic to test block->numSuccessors() == 1. > > We don't really care about the kind of ControlValue for changing the flow > > graph. > > Currently it doesn't matter - the only other way to get one successor is to > have a Switch with no cases and just a fall-through, but the solution there > is to make strength-reduction fix those. > > As for future opcodes, testing for one successor is more generic but maybe > less safe. Probably if we added a new opcode that had a single successor it > would only be because that opcode did something special, which would make it > illegal to do what this phase does. I mean, I guess we could test if > numSuccessors is 1 and effects() is empty, but that feels like a bit much. > > > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:146 > > > + fixSSA(m_proc); > > > > If I am not mistaken: > > > > This is only called if (changed == true). If all the blocks in "candidates" > > are reached by a branch instead of a jump, their values would be demoted but > > never put back in SSA. > > D'oh! Good catch. Obviously, after we call demoteValues() we need to > commit to calling fixSSA(). Fixed. > > > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:202 > > > + void foldPathConstants() > > > > Clever. > > > > Shouldn't it have its own phase? It seems unrelated to tail duplication. > > That's a good idea. I'll do that. Done. > > > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:221 > > > + if (override.block->predecessor(0) != from) > > > > That's odd. I would think this does not happen since the previous phase does > > resetReachability(). > > Yeah, this should be an assertion. Fixed. > > > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:235 > > > + if (value->index() >= 150) > > > + return; > > > + if (value->index() < 50) > > > + return; > > > > You are losing me there. Why does this depends on the index? > > Ha! This was debug code. This machinery was failing some test. This would > explain why my deltablue speed-up went down from 20% to 13%. ;-) > > I'll retest everything after removing this garbage. Removed. Haven't tested it yet, though. > > > > > > Source/JavaScriptCore/b3/B3FixSSA.cpp:248 > > > + mapping.clear(); > > > > Why not declare mapping in the loop? > > Because I'm fantasizing about a future utopia where HashMap::clear() doesn't > always free the backing store. > > > > > > Source/JavaScriptCore/b3/air/AirCode.h:317 > > > + void forAllTmps(Arg::Type type, const Callback& callback) const > > > > This does not seem used anywhere. > > I'll kill it. Dead. > > > > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:104 > > > + if (m_unspillableTmps.contains(toSpill)) > > > > Are you sure that's not a regression? Adding to the m_spillWorklist is not > > uncommon. > > > > I fixed the same bug today, somewhat differently. > > I'll revert my changes to IRC so long as doing so doesn't break anything. > This change did speed up testComplex with taildup. Part of that was to actually fix a bug - selectSpill() would sometimes crash in score() if it was passed an unspillable. I'm fine with you basically rolling out my version of this and doing your change instead.
Filip Pizlo
Comment 24 2016-01-20 17:46:00 PST
> > > > > > > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:104 > > > > + if (m_unspillableTmps.contains(toSpill)) > > > > > > Are you sure that's not a regression? Adding to the m_spillWorklist is not > > > uncommon. > > > > > > I fixed the same bug today, somewhat differently. > > > > I'll revert my changes to IRC so long as doing so doesn't break anything. > > This change did speed up testComplex with taildup. > > Part of that was to actually fix a bug - selectSpill() would sometimes crash > in score() if it was passed an unspillable. I'm fine with you basically > rolling out my version of this and doing your change instead. I take this back. Yours already landed. I *think* that my solution is a bit better, see https://bugs.webkit.org/show_bug.cgi?id=153287#c9
Benjamin Poulain
Comment 25 2016-01-20 17:52:04 PST
(In reply to comment #24) > > > > > > > > > > > > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:104 > > > > > + if (m_unspillableTmps.contains(toSpill)) > > > > > > > > Are you sure that's not a regression? Adding to the m_spillWorklist is not > > > > uncommon. > > > > > > > > I fixed the same bug today, somewhat differently. > > > > > > I'll revert my changes to IRC so long as doing so doesn't break anything. > > > This change did speed up testComplex with taildup. > > > > Part of that was to actually fix a bug - selectSpill() would sometimes crash > > in score() if it was passed an unspillable. I'm fine with you basically > > rolling out my version of this and doing your change instead. > > I take this back. Yours already landed. I *think* that my solution is a > bit better, see https://bugs.webkit.org/show_bug.cgi?id=153287#c9 Okay with me if you want to revert my fix and land yours. I think your fix is cleaner. I am very very paranoid about the perf of IRC but I am not sure which version is best. My assumption was that selectSpill() is uncommon and take more cost if it make hot code simpler. Your version will make the spillWorklist smaller which may be more beneficial since most elements of spillWorklist are removed without ever going to selectSpill().
Filip Pizlo
Comment 26 2016-01-20 18:42:31 PST
> > > > > > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:235 > > > > + if (value->index() >= 150) > > > > + return; > > > > + if (value->index() < 50) > > > > + return; > > > > > > You are losing me there. Why does this depends on the index? > > > > Ha! This was debug code. This machinery was failing some test. This would > > explain why my deltablue speed-up went down from 20% to 13%. ;-) > > > > I'll retest everything after removing this garbage. > > Removed. Haven't tested it yet, though. Ha! It's an even bigger speed-up! 2.2% faster on Octane.
Filip Pizlo
Comment 27 2016-01-20 18:44:50 PST
Created attachment 269412 [details] patch for landing
WebKit Commit Bot
Comment 28 2016-01-20 18:46:56 PST
Attachment 269412 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3Value.h:219: The parameter name "functor" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ValueKey.h:88: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1598: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3FixSSA.cpp:141: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3FixSSA.cpp:146: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9091: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9105: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9106: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9106: 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/testb3.cpp:9107: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9134: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9162: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9176: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9177: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9178: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9178: 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/testb3.cpp:9179: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9180: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] Total errors found: 18 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.
Filip Pizlo
Comment 29 2016-01-20 19:13:54 PST
Geoffrey Garen
Comment 30 2016-01-21 10:41:59 PST
> > > > > Source/JavaScriptCore/b3/B3DuplicateTails.cpp:235 > > > > > + if (value->index() >= 150) > > > > > + return; > > > > > + if (value->index() < 50) > > > > > + return; > > > > > > > > You are losing me there. Why does this depends on the index? > > > > > > Ha! This was debug code. This machinery was failing some test. This would > > > explain why my deltablue speed-up went down from 20% to 13%. ;-) > > > > > > I'll retest everything after removing this garbage. > > > > Removed. Haven't tested it yet, though. > > Ha! It's an even bigger speed-up! 2.2% faster on Octane. LOL, patch review for the win.
Note You need to log in before you can comment on or make changes to this bug.