Bug 151214 - CheckAdd/Mul should have commutativity optimizations in B3->Air lowering
Summary: CheckAdd/Mul should have commutativity optimizations in B3->Air lowering
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:
Depends on: 151213 151315
Blocks: 150279 151335
  Show dependency treegraph
 
Reported: 2015-11-12 12:42 PST by Filip Pizlo
Modified: 2015-11-17 14:31 PST (History)
14 users (show)

See Also:


Attachments
work in progress (43.60 KB, patch)
2015-11-16 16:11 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (84.28 KB, patch)
2015-11-16 21:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (84.52 KB, patch)
2015-11-16 21:42 PST, 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-11-12 12:42:41 PST
Stuff.
Comment 1 Filip Pizlo 2015-11-16 12:24:21 PST
I think that the right story for commutativity is just to allow B3 to commute CheckAdd/CheckMul.  If you are a client of B3 and you want to emit:

    CheckAdd(@x, @y)

And you want your generator to know what @x and @y are *before* commutation, you should do:

    CheckAdd(@x, @y, @x, @y)

We might commute the first two args since those are relevant to the math, but we won't commute the stackmap arguments.
Comment 2 Filip Pizlo 2015-11-16 16:10:47 PST
While doing this and talking to Ben, I realized that we need to have the notion of a LateUse: a Use that happens after all relevant Def's, i.e. it appears like it's a Use on the instruction following this one.  We need this to handle:

- CheckAdd(@x, @x)
- CheckMul(@x, @y) where the register allocator uses the same register for the result as for the inputs
Comment 3 Filip Pizlo 2015-11-16 16:11:17 PST
Created attachment 265634 [details]
work in progress
Comment 4 Filip Pizlo 2015-11-16 21:35:26 PST
Created attachment 265661 [details]
the patch
Comment 5 Filip Pizlo 2015-11-16 21:42:14 PST
Created attachment 265662 [details]
the patch
Comment 6 WebKit Commit Bot 2015-11-16 21:44:36 PST
Attachment 265662 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/testb3.cpp:3216:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3219:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3250:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3251:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3254:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3285:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3286:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3322:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3354:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3426:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3429:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3461:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3462:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3499:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3536:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3607:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3636:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3670:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3709:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3710:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3754:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3755:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3758:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3788:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:56:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:137:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.h:57:  The parameter name "role" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 28 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Benjamin Poulain 2015-11-17 03:12:39 PST
I think I got the allocator bug: https://bugs.webkit.org/show_bug.cgi?id=151345
Comment 8 Geoffrey Garen 2015-11-17 13:50:54 PST
Comment on attachment 265662 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265662&action=review

r=me

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:573
> -        
> +

Revert-o.
Comment 9 Filip Pizlo 2015-11-17 14:18:50 PST
Comment on attachment 265662 [details]
the patch

Clearing cq so that I can land it myself.
Comment 10 Filip Pizlo 2015-11-17 14:31:58 PST
Landed in http://trac.webkit.org/changeset/192540