WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 150748
B3::reduceStrength's DCE should be more agro and less wrong
https://bugs.webkit.org/show_bug.cgi?id=150748
Summary
B3::reduceStrength's DCE should be more agro and less wrong
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.
Top of Page
Format For Printing
XML
Clone This Bug