Bug 151214 – CheckAdd/Mul should have commutativity optimizations in B3->Air lowering

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Stuff.

[Filip Pizlo]

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.

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 265634 [details]
work in progress

[Filip Pizlo]

Created attachment 265661 [details]
the patch

[Filip Pizlo]

Created attachment 265662 [details]
the patch

[WebKit Commit Bot]

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.

[Benjamin Poulain]

I think I got the allocator bug: https://bugs.webkit.org/show_bug.cgi?id=151345

[Geoffrey Garen]

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.

[Filip Pizlo]

Comment on attachment 265662 [details]
the patch

Clearing cq so that I can land it myself.

[Filip Pizlo]

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