Bug 150748 - B3::reduceStrength's DCE should be more agro and less wrong
Summary: B3::reduceStrength's DCE should be more agro and less wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
: 150751 (view as bug list)
Depends on:
Blocks: 150507
  Show dependency treegraph
 
Reported: 2015-10-30 19:10 PDT by Filip Pizlo
Modified: 2015-11-01 15:38 PST (History)
12 users (show)

See Also:


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
sbarati: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 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.
Comment 1 Filip Pizlo 2015-10-30 19:16:05 PDT
Created attachment 264460 [details]
the patch
Comment 2 Filip Pizlo 2015-10-30 19:20:41 PDT
Created attachment 264461 [details]
the patch
Comment 3 Saam Barati 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.
Comment 4 Filip Pizlo 2015-10-31 11:29:34 PDT
Created attachment 264479 [details]
the patch

Revised patch, with bug fix.
Comment 5 Filip Pizlo 2015-10-31 11:39:13 PDT
Comment on attachment 264479 [details]
the patch

Never mind, it's still broken.
Comment 6 Filip Pizlo 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.
Comment 7 Filip Pizlo 2015-10-31 15:50:52 PDT
Created attachment 264490 [details]
the patch
Comment 8 WebKit Commit Bot 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.
Comment 9 Filip Pizlo 2015-10-31 16:14:42 PDT
*** Bug 150751 has been marked as a duplicate of this bug. ***
Comment 10 Filip Pizlo 2015-10-31 16:30:10 PDT
Created attachment 264491 [details]
the patch
Comment 11 WebKit Commit Bot 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.
Comment 12 Geoffrey Garen 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.
Comment 13 Filip Pizlo 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.
Comment 14 Filip Pizlo 2015-11-01 15:38:31 PST
Landed in http://trac.webkit.org/changeset/191865