Bug 196325 – B3ReduceStrength should know that Mul distributes over Add and Sub

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

...

[Robin Morisset]

Created attachment 366127 [details]
Patch

Note: there are other patterns involving Mul that I plan to add in a separate patch. For example Add (x, Mul (x, y)) => Mul (x, y+1).

[Michael Saboff]

Comment on attachment 366127 [details]
Patch

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

r=me with comment

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2439
> -            replaceWithNew<Value>(BitAnd, m_value->origin(), bitOp, x1);
> +            replaceWithNew<Value>(BitAnd, m_value->origin(), x1, bitOp);

This change isn't described in the ChangeLog.

[Robin Morisset]

Created attachment 366325 [details]
Patch

Same patch as before, except for one more line in the changelog.

[Robin Morisset]

Thanks for the review!

I added a line in the changelog about this (fairly trivial) change.

(In reply to Michael Saboff from comment #2)
> Comment on attachment 366127 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366127&action=review
> 
> r=me with comment
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2439
> > -            replaceWithNew<Value>(BitAnd, m_value->origin(), bitOp, x1);
> > +            replaceWithNew<Value>(BitAnd, m_value->origin(), x1, bitOp);
> 
> This change isn't described in the ChangeLog.

[WebKit Commit Bot]

Comment on attachment 366325 [details]
Patch

Rejecting attachment 366325 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 366325, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=366325&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=196325&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 366325 from bug 196325.
Fetching: https://bugs.webkit.org/attachment.cgi?id=366325
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Parsed 3 diffs from patch file(s).
patching file Source/JavaScriptCore/ChangeLog
patch: **** malformed patch at line 37:  2019-03-25  Gyuyoung Kim  <[e-mail]>

patching file Source/JavaScriptCore/b3/B3ReduceStrength.cpp
patching file Source/JavaScriptCore/b3/testb3.cpp
Hunk #4 succeeded at 17070 (offset 45 lines).
Hunk #5 succeeded at 17155 (offset 45 lines).
Hunk #6 succeeded at 17241 (offset 45 lines).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/11707553

[Robin Morisset]

Created attachment 366326 [details]
Patch

Fixed a patch format issue.

[WebKit Commit Bot]

Comment on attachment 366326 [details]
Patch

Clearing flags on attachment: 366326

Committed r243670: <https://trac.webkit.org/changeset/243670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49441650>

[Michael Catanzaro]

Robin, GCC found a bug here. Look:

[2656/4718] Building CXX object Source/JavaScriptCore/CMa...ScriptCore/unified-sources/UnifiedSource-23a5fd0e-7.cpp.o
In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-23a5fd0e-7.cpp:6:
../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp: In member function ‘bool JSC::B3::{anonymous}::ReduceStrength::handleMulDistributivity()’:
../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2348:20: warning: variable ‘x1’ set but not used [-Wunused-but-set-variable]
 2348 |             Value* x1 = nullptr;
      |                    ^~
../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2349:20: warning: variable ‘x2’ set but not used [-Wunused-but-set-variable]
 2349 |             Value* x2 = nullptr;
      |                    ^~
../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2350:20: warning: variable ‘x3’ set but not used [-Wunused-but-set-variable]
 2350 |             Value* x3 = nullptr;
      |                    ^~

Indeed, the redeclared x1, x2, and x3 all go out of scope before they're ever used. I think they're being shadowed by mistake, right? Proposed solution:

diff --git a/Source/JavaScriptCore/b3/B3ReduceStrength.cpp b/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
index 9e6215942f3..7e22c29d6d4 100644
--- a/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
+++ b/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
@@ -2345,9 +2345,6 @@ private:
         Value* x2 = nullptr;
         Value* x3 = nullptr;
         if (m_value->child(0)->opcode() == Mul && m_value->child(1)->opcode() == Mul) {
-            Value* x1 = nullptr;
-            Value* x2 = nullptr;
-            Value* x3 = nullptr;
             if (m_value->child(0)->child(0) == m_value->child(1)->child(0)) {
                 // Op(Mul(x1, x2), Mul(x1, x3))
                 x1 = m_value->child(0)->child(0);

[Robin Morisset]

Indeed, this is a clear bug, probably an accidental copy-paste.
Thanks for the report, I will send a patch as soon as my current compile is over.

(In reply to Michael Catanzaro from comment #10)
> Robin, GCC found a bug here. Look:
> 
> [2656/4718] Building CXX object
> Source/JavaScriptCore/CMa...ScriptCore/unified-sources/UnifiedSource-
> 23a5fd0e-7.cpp.o
> In file included from
> DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-23a5fd0e-7.cpp:6:
> ../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp: In member function
> ‘bool JSC::B3::{anonymous}::ReduceStrength::handleMulDistributivity()’:
> ../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2348:20: warning:
> variable ‘x1’ set but not used [-Wunused-but-set-variable]
>  2348 |             Value* x1 = nullptr;
>       |                    ^~
> ../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2349:20: warning:
> variable ‘x2’ set but not used [-Wunused-but-set-variable]
>  2349 |             Value* x2 = nullptr;
>       |                    ^~
> ../../Source/JavaScriptCore/b3/B3ReduceStrength.cpp:2350:20: warning:
> variable ‘x3’ set but not used [-Wunused-but-set-variable]
>  2350 |             Value* x3 = nullptr;
>       |                    ^~
> 
> Indeed, the redeclared x1, x2, and x3 all go out of scope before they're
> ever used. I think they're being shadowed by mistake, right? Proposed
> solution:
> 
> diff --git a/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
> b/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
> index 9e6215942f3..7e22c29d6d4 100644
> --- a/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
> +++ b/Source/JavaScriptCore/b3/B3ReduceStrength.cpp
> @@ -2345,9 +2345,6 @@ private:
>          Value* x2 = nullptr;
>          Value* x3 = nullptr;
>          if (m_value->child(0)->opcode() == Mul &&
> m_value->child(1)->opcode() == Mul) {
> -            Value* x1 = nullptr;
> -            Value* x2 = nullptr;
> -            Value* x3 = nullptr;
>              if (m_value->child(0)->child(0) == m_value->child(1)->child(0))
> {
>                  // Op(Mul(x1, x2), Mul(x1, x3))
>                  x1 = m_value->child(0)->child(0);

[Saam Barati]

Can you add a test too?

[Robin Morisset]

(In reply to Saam Barati from comment #12)
> Can you add a test too?

There were already a lot of tests that the B3 optimizations don't have soundness issues (including some that I added for this optimization in particular). What would be needed to find this kind of case where an optimization is dead/never trigger is a different kind of thing. I could add some microbenchmark, but currently most of the peephole optimizations don't have one.
Maybe another option would be some logging of which optimizations triggers (as a compile time option), and verifying on JetStream2 + microbenchmarks that all optimizations trigger at least twice. It would have the benefit of requiring less work 
and could give us hints of what optimizations matter most/should be improved.

[Robin Morisset]

> that all optimizations trigger at least twice.

I meant *once*.

[Robin Morisset]

Created attachment 366767 [details]
PatchForFix

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 366767 [details]:

media/W3C/video/events/event_progress.html bug 196637 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 366767 [details]
PatchForFix

Clearing flags on attachment: 366767

Committed r243918: <https://trac.webkit.org/changeset/243918>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.