Bug 150748

Summary:

B3::reduceStrength's DCE should be more agro and less wrong

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, cmarcelo, commit-queue, ggaren, mark.lam, mhahnenb, msaboff, nrotem, oliver, saam, sam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

150507

Attachments:

Description	Flags
the patch	none
the patch	saam: review+
the patch	fpizlo: review-
work in progress	none
the patch	none
the patch	ggaren: review+

Filip Pizlo

Reported 2015-10-30 19:10:42 PDT

There's just no good reason not to do a more aggressive DCE, where we presume everything dead and fixpoint to find live things. It's especially easy when we use the graph helpers I extracted into WTF.

Attachments
the patch (6.25 KB, patch) 2015-10-30 19:16 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (7.24 KB, patch) 2015-10-30 19:20 PDT, Filip Pizlo	saam: review+	Details Formatted Diff Diff
the patch (8.30 KB, patch) 2015-10-31 11:29 PDT, Filip Pizlo	fpizlo: review-	Details Formatted Diff Diff
work in progress (33.07 KB, patch) 2015-10-31 13:24 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (37.45 KB, patch) 2015-10-31 15:50 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (52.85 KB, patch) 2015-10-31 16:30 PDT, Filip Pizlo	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2015-10-30 19:16:05 PDT

Created attachment 264460 [details] the patch

Filip Pizlo

Comment 2 2015-10-30 19:20:41 PDT

Created attachment 264461 [details] the patch

Saam Barati

Comment 3 2015-10-31 01:45:08 PDT

Comment on attachment 264461 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=264461&action=review r=me You're right, writing this kind of DCE is super easy with that data structure. > Source/JavaScriptCore/ChangeLog:3 > + B3::reduceStrength's DCE should be more agro When I read agro I can't help but think of "agriculture" > Source/JavaScriptCore/ChangeLog:7 > + It's worth adding a description here.

Filip Pizlo

Comment 4 2015-10-31 11:29:34 PDT

Created attachment 264479 [details] the patch Revised patch, with bug fix.

Filip Pizlo

Comment 5 2015-10-31 11:39:13 PDT

Comment on attachment 264479 [details] the patch Never mind, it's still broken.

Filip Pizlo

Comment 6 2015-10-31 13:24:05 PDT

Created attachment 264487 [details] work in progress To really get a feeling for the bug, I had to use my work-in-progress "complex test" that I'm going to use to compare compile times to LLVM. So, I think this changeset should just include that.

Filip Pizlo

Comment 7 2015-10-31 15:50:52 PDT

Created attachment 264490 [details] the patch

WebKit Commit Bot

Comment 8 2015-10-31 15:52:48 PDT

Attachment 264490 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:59: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:38: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:43: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:45: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:46: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:50: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:52: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:63: bitwise_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:98: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/ReducedFTL/ComplexTest.cpp:210: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 16 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 9 2015-10-31 16:14:42 PDT

*** Bug 150751 has been marked as a duplicate of this bug. ***

Filip Pizlo

Comment 10 2015-10-31 16:30:10 PDT

Created attachment 264491 [details] the patch

WebKit Commit Bot

Comment 11 2015-10-31 16:32:33 PDT

Attachment 264491 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:59: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:38: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:43: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:45: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:46: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:48: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:50: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:52: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:63: bitwise_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Tools/ReducedFTL/ComplexTest.cpp:98: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Tools/ReducedFTL/ComplexTest.cpp:210: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 16 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Geoffrey Garen

Comment 12 2015-11-01 12:57:25 PST

Comment on attachment 264491 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=264491&action=review r=me > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:339 > + // Can only shift by ecx, so we do some swapping of we see anything else. of => if > Source/JavaScriptCore/runtime/Options.h:340 > + v(bool, measurePhaseTimings, false, nullptr) \ How about "logB3PhaseTimes" or "useB3TimeLogging"? "Phase" alone doesn't provide context.

Filip Pizlo

Comment 13 2015-11-01 13:20:07 PST

(In reply to comment #12) > Comment on attachment 264491 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264491&action=review > > r=me > > > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:339 > > + // Can only shift by ecx, so we do some swapping of we see anything else. > > of => if Fixed! > > > Source/JavaScriptCore/runtime/Options.h:340 > > + v(bool, measurePhaseTimings, false, nullptr) \ > > How about "logB3PhaseTimes" or "useB3TimeLogging"? > > "Phase" alone doesn't provide context. I used logB3PhaseTimes.

Filip Pizlo

Comment 14 2015-11-01 15:38:31 PST

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

Note You need to log in before you can comment on or make changes to this bug.