151321 – B3 should be be clever about choosing which child to reuse for result in two-operand commutative operations

RESOLVED FIXED 151321

B3 should be be clever about choosing which child to reuse for result in two-operand commutative operations

https://bugs.webkit.org/show_bug.cgi?id=151321

Summary B3 should be be clever about choosing which child to reuse for result in two-...

Filip Pizlo

Reported 2015-11-16 13:19:53 PST

If we have: Int32 @3 = Add(@1, @2) and we know that @1 will die, then we should lower this to: Move %tmp1, %tmp3 Add32 %tmp2, %tmp3 But if we know that @2 will die, then we should lower this to: Move %tmp2, %tmp3 Add32 %tmp1, %tmp3

Attachments
I wrote some code (8.65 KB, patch) 2015-11-28 11:50 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
almost happy with it (10.18 KB, patch) 2015-11-29 16:06 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (29.38 KB, patch) 2015-11-29 17:29 PST, Filip Pizlo	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-yosemite (1.08 MB, application/zip) 2015-11-29 18:14 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.02 MB, application/zip) 2015-11-29 18:15 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews117 for mac-yosemite (1.12 MB, application/zip) 2015-11-29 18:16 PST, Build Bot	no flags	Details
the patch (28.79 KB, patch) 2015-11-30 10:36 PST, Filip Pizlo	ggaren: review+	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2015-11-28 11:50:56 PST

Created attachment 266214 [details] I wrote some code

Filip Pizlo

Comment 2 2015-11-29 16:06:13 PST

Created attachment 266227 [details] almost happy with it This is a regression on imaging-gaussian-blur because it commutes something that really should not have been commuted. I'm thinking about what to do.

Filip Pizlo

Comment 3 2015-11-29 17:29:31 PST

Created attachment 266229 [details] the patch

Filip Pizlo

Comment 4 2015-11-29 17:30:32 PST

Comment on attachment 266229 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=266229&action=review > Source/JavaScriptCore/dfg/DFGCommon.h:41 > -#define FTL_USES_B3 0 > +#define FTL_USES_B3 1 I will revert, I promise!

WebKit Commit Bot

Comment 5 2015-11-29 17:32:01 PST

Attachment 266229 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3PhiChildren.h:154: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Build Bot

Comment 6 2015-11-29 18:14:09 PST

Comment on attachment 266229 [details] the patch Attachment 266229 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/494819 Number of test failures exceeded the failure limit.

Build Bot

Comment 7 2015-11-29 18:14:12 PST

Created attachment 266231 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5

Build Bot

Comment 8 2015-11-29 18:15:33 PST

Comment on attachment 266229 [details] the patch Attachment 266229 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/494816 Number of test failures exceeded the failure limit.

Build Bot

Comment 9 2015-11-29 18:15:35 PST

Created attachment 266232 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Build Bot

Comment 10 2015-11-29 18:16:05 PST

Comment on attachment 266229 [details] the patch Attachment 266229 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/494812 Number of test failures exceeded the failure limit.

Build Bot

Comment 11 2015-11-29 18:16:07 PST

Created attachment 266233 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Filip Pizlo

Comment 12 2015-11-30 10:36:16 PST

Created attachment 266251 [details] the patch

WebKit Commit Bot

Comment 13 2015-11-30 10:37:23 PST

Attachment 266251 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3PhiChildren.h:154: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Geoffrey Garen

Comment 14 2015-11-30 12:14:21 PST

Comment on attachment 266251 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=266251&action=review r=me > Source/JavaScriptCore/ChangeLog:8 > + When lowering a commutative operation two a two-operand instruction, you have a choice of which to > Source/JavaScriptCore/b3/B3LowerToAir.cpp:542 > + // then we can flip things around to held coalescing, which then kills the move instruction. help > Source/JavaScriptCore/b3/B3PhiChildren.h:109 > + UpsilonCollection() > + { > + } Do we use this null initializer? I couldn't find its use. If we do use it, we should probably null-initialize our data members.

Filip Pizlo

Comment 15 2015-11-30 12:18:55 PST

(In reply to comment #14) > Comment on attachment 266251 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266251&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:8 > > + When lowering a commutative operation two a two-operand instruction, you have a choice of which > > to > > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:542 > > + // then we can flip things around to held coalescing, which then kills the move instruction. > > help > > > Source/JavaScriptCore/b3/B3PhiChildren.h:109 > > + UpsilonCollection() > > + { > > + } > > Do we use this null initializer? I couldn't find its use. > > If we do use it, we should probably null-initialize our data members. I added the new-style { } initializers. I like it when these classes have a null initializer because then you can use them more freely, like: PhiChildren::UpsilonCollection x; if (things) x = phis[index]; else x = otherPhis[index];

Filip Pizlo

Comment 16 2015-11-30 15:20:23 PST

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

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component JavaScriptCore

Assignee

Filip Pizlo

Reported

2015-11-16 13:19 PST

Modified

2015-11-30 15:20 PST History

CC List

13 users Show

URL

Keywords

Depends on

Blocks

150279

Dependencies

tree graph