Bug 199251 – Add b3 macro lowering for CheckMul on arm64

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

[Justin Michaud]

CheckMul emits quite a few instructions on arm64, so we should try lowering it in b3.

[Justin Michaud]

Created attachment 372977 [details]
Patch

[Saam Barati]

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

[Saam Barati]

Also, I doubt this is faster on x86, but let's just verify that hypothesis.

[Saam Barati]

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

[Justin Michaud]

Created attachment 373149 [details]
Patch

[Justin Michaud]

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?

[Robin Morisset]

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?

[Robin Morisset]

(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.

[Justin Michaud]

Created attachment 373155 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 373155 [details]
Patch

Clearing flags on attachment: 373155

Committed r246946: <https://trac.webkit.org/changeset/246946>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52359122>

[Ryan Haddad]

Reverted r246946 for reason:

Caused JSC test crashes on arm64

Committed r247011: <https://trac.webkit.org/changeset/247011>

[Justin Michaud]

Created attachment 373926 [details]
Patch

[Robin Morisset]

Comment on attachment 373926 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 373926 [details]
Patch

Clearing flags on attachment: 373926

Committed r247363: <https://trac.webkit.org/changeset/247363>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.