Bug 196325 - B3ReduceStrength should know that Mul distributes over Add and Sub
Summary: B3ReduceStrength should know that Mul distributes over Add and Sub
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-27 16:16 PDT by Robin Morisset
Modified: 2019-04-05 11:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch (14.76 KB, patch)
2019-03-27 16:26 PDT, Robin Morisset
msaboff: review+
Details | Formatted Diff | Diff
Patch (15.02 KB, patch)
2019-03-29 16:36 PDT, Robin Morisset
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (14.81 KB, patch)
2019-03-29 16:41 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
PatchForFix (1.44 KB, patch)
2019-04-04 16:13 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-03-27 16:16:31 PDT
...
Comment 1 Robin Morisset 2019-03-27 16:26:00 PDT
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).
Comment 2 Michael Saboff 2019-03-29 16:18:09 PDT
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.
Comment 3 Robin Morisset 2019-03-29 16:36:34 PDT
Created attachment 366325 [details]
Patch

Same patch as before, except for one more line in the changelog.
Comment 4 Robin Morisset 2019-03-29 16:37:08 PDT
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.
Comment 5 WebKit Commit Bot 2019-03-29 16:37:47 PDT
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 commit-queue@webkit.org...
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  <gyuyoung.kim@webkit.org>

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
Comment 6 Robin Morisset 2019-03-29 16:41:57 PDT
Created attachment 366326 [details]
Patch

Fixed a patch format issue.
Comment 7 WebKit Commit Bot 2019-03-29 17:21:49 PDT
Comment on attachment 366326 [details]
Patch

Clearing flags on attachment: 366326

Committed r243670: <https://trac.webkit.org/changeset/243670>
Comment 8 WebKit Commit Bot 2019-03-29 17:21:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-03-29 17:22:38 PDT
<rdar://problem/49441650>
Comment 10 Michael Catanzaro 2019-04-04 13:57:24 PDT
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);
Comment 11 Robin Morisset 2019-04-04 15:35:38 PDT
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);
Comment 12 Saam Barati 2019-04-04 15:53:49 PDT
Can you add a test too?
Comment 13 Robin Morisset 2019-04-04 16:01:02 PDT
(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.
Comment 14 Robin Morisset 2019-04-04 16:01:49 PDT
> that all optimizations trigger at least twice.

I meant *once*.
Comment 15 Robin Morisset 2019-04-04 16:13:03 PDT
Created attachment 366767 [details]
PatchForFix
Comment 16 WebKit Commit Bot 2019-04-04 17:07:58 PDT
The commit-queue encountered the following flaky tests while processing attachment 366767 [details]:

media/W3C/video/events/event_progress.html bug 196637 (author: pilgrim@chromium.org)
The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2019-04-04 17:08:37 PDT
Comment on attachment 366767 [details]
PatchForFix

Clearing flags on attachment: 366767

Committed r243918: <https://trac.webkit.org/changeset/243918>
Comment 18 WebKit Commit Bot 2019-04-04 17:08:38 PDT
All reviewed patches have been landed.  Closing bug.