Bug 199251 - Add b3 macro lowering for CheckMul on arm64
Summary: Add b3 macro lowering for CheckMul on arm64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-26 17:41 PDT by Justin Michaud
Modified: 2019-07-11 14:08 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.16 KB, patch)
2019-06-26 17:42 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (17.16 KB, patch)
2019-06-28 14:12 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (17.20 KB, patch)
2019-06-28 15:25 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (17.40 KB, patch)
2019-07-11 10:17 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-06-26 17:41:23 PDT
CheckMul emits quite a few instructions on arm64, so we should try lowering it in b3.
Comment 1 Justin Michaud 2019-06-26 17:42:48 PDT
Created attachment 372977 [details]
Patch
Comment 2 Saam Barati 2019-06-26 20:17:23 PDT
Comment on attachment 372977 [details]
Patch

r=me

Can you please also add some B3 tests for this to ensure we do the right thing? in testb3
Comment 3 Saam Barati 2019-06-26 20:17:51 PDT
Also, I doubt this is faster on x86, but let's just verify that hypothesis.
Comment 4 Saam Barati 2019-06-26 20:18:36 PDT
Comment on attachment 372977 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Add b3 macro lowering for CheckMul on arm

arm => arm64

> Source/JavaScriptCore/ChangeLog:8
> +        Lower CheckMul for 32-bit arguments on arm into a mul and then an overflow check.

nit: Also say what speedup you got on the microbenchmark

arm => arm64
Comment 5 Justin Michaud 2019-06-28 14:12:47 PDT
Created attachment 373149 [details]
Patch
Comment 6 Justin Michaud 2019-06-28 14:14:54 PDT
Comment on attachment 373149 [details]
Patch

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

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2616
> +                } else if (m_value->child(1)->isRepresentableAs<int32_t>()) {

Is there ever a case where this might be locked?
Comment 7 Robin Morisset 2019-06-28 14:21:03 PDT
Comment on attachment 373149 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        - Lower CheckMul for 33-bit arguments on arm64 into a mul and then an overflow check.

Did you mean 32-bit here?
Comment 8 Robin Morisset 2019-06-28 14:22:30 PDT
(In reply to Justin Michaud from comment #6)
> Comment on attachment 373149 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373149&action=review
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2616
> > +                } else if (m_value->child(1)->isRepresentableAs<int32_t>()) {
> 
> Is there ever a case where this might be locked?

Maybe. I've not touched the instruction selection yet; it would probably be safer to check that it is not locked. It does not cost much if it never is locked.
Comment 9 Justin Michaud 2019-06-28 15:25:07 PDT
Created attachment 373155 [details]
Patch
Comment 10 WebKit Commit Bot 2019-06-28 15:27:24 PDT
Comment on attachment 373155 [details]
Patch

Rejecting attachment 373155 [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', 'validate-changelog', '--check-oops', '--non-interactive', 373155, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Robin Morisset found in /Volumes/Data/EWS/WebKit/JSTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: https://webkit-queues.webkit.org/results/12607016
Comment 11 WebKit Commit Bot 2019-06-28 16:19:17 PDT
Comment on attachment 373155 [details]
Patch

Clearing flags on attachment: 373155

Committed r246946: <https://trac.webkit.org/changeset/246946>
Comment 12 WebKit Commit Bot 2019-06-28 16:19:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-06-28 16:20:18 PDT
<rdar://problem/52359122>
Comment 14 Ryan Haddad 2019-07-01 09:59:39 PDT
Reverted r246946 for reason:

Caused JSC test crashes on arm64

Committed r247011: <https://trac.webkit.org/changeset/247011>
Comment 15 Justin Michaud 2019-07-11 10:17:56 PDT
Created attachment 373926 [details]
Patch
Comment 16 Robin Morisset 2019-07-11 13:11:01 PDT
Comment on attachment 373926 [details]
Patch

r=me
Comment 17 WebKit Commit Bot 2019-07-11 14:08:42 PDT
Comment on attachment 373926 [details]
Patch

Clearing flags on attachment: 373926

Committed r247363: <https://trac.webkit.org/changeset/247363>
Comment 18 WebKit Commit Bot 2019-07-11 14:08:44 PDT
All reviewed patches have been landed.  Closing bug.