Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JSC] Refine B3 and masm tests for EXTR pattern detection and application #23820

Merged
merged 1 commit into from
Feb 6, 2024
Merged

[JSC] Refine B3 and masm tests for EXTR pattern detection and application #23820

merged 1 commit into from
Feb 6, 2024

Conversation

hyjorc1
Copy link
Contributor

@hyjorc1 hyjorc1 commented Feb 4, 2024 •
edited by webkit-early-warning-system
Loading

084df28

[JSC] Refine B3 and masm tests for EXTR pattern detection and application
https://bugs.webkit.org/show_bug.cgi?id=264984
rdar://118532295

Reviewed by Keith Miller.

Previously we introduced the pattern detection and application
of EXTR when B3 is lowering to AIR. This patch does two things:
1. Refine the corresponding B3 and masm tests for EXTR.
2. Add a sanity check and more comments for EXTR pattern detection and application in B3.

* Source/JavaScriptCore/assembler/testmasm.cpp:
(JSC::testExtractRegister32):
(JSC::testExtractRegister64):
* Source/JavaScriptCore/b3/B3LowerToAir.cpp:
* Source/JavaScriptCore/b3/testb3_2.cpp:
(testExtractRegister32):
(testExtractRegister64):

Canonical link: https://commits.webkit.org/274149@main

2bec550

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ❌ 🧪 jsc-armv7-tests
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@hyjorc1 hyjorc1 self-assigned this Feb 4, 2024
@hyjorc1 hyjorc1 added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Feb 4, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 4, 2024 •
edited
Loading

EWS run on previous version of this PR (hash c5a6b63)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ⏳ 🛠 wincairo
✅ 🛠 ios-sim   🛠 mac-AS-debug ❌ 🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac ❌ 🧪 api-wpe
⏳ 🧪 webkitpy   🧪 ios-wk2-wpt   🧪 mac-wk1   🛠 gtk
⏳ 🛠 🧪 jsc   🧪 api-ios ⏳ 🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv   🧪 mac-AS-debug-wk2   🧪 api-gtk
⏳ 🛠 tv-sim   🛠 jsc-armv7
⏳ 🛠 watch   🧪 jsc-armv7-tests
⏳ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 4, 2024
@hyjorc1 hyjorc1 removed the merging-blocked Applied to prevent a change from being merged label Feb 4, 2024
@hyjorc1 hyjorc1 force-pushed the eng/JSC-refine-EXTR-pattern-detection-tests-for-b3-and-masm branch from c5a6b63 to 5894357 Compare February 4, 2024 01:12
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 4, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 5894357)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac   🛠 wpe ⏳ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
  🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac   🧪 api-wpe
⏳ 🧪 webkitpy   🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
⏳ 🛠 🧪 jsc   🧪 api-ios   🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv   🧪 mac-AS-debug-wk2   🧪 api-gtk
⏳ 🛠 tv-sim ✅ 🛠 jsc-armv7
⏳ 🛠 watch   🧪 jsc-armv7-tests
⏳ 🛠 watch-sim

@hyjorc1 hyjorc1 force-pushed the eng/JSC-refine-EXTR-pattern-detection-tests-for-b3-and-masm branch from 5894357 to 3959022 Compare February 4, 2024 01:15
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 4, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 3959022)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ❌ 🧪 jsc-armv7-tests
✅ 🛠 watch-sim

@hyjorc1 hyjorc1 marked this pull request as ready for review February 4, 2024 01:16
@hyjorc1 hyjorc1 requested a review from a team as a code owner February 4, 2024 01:16
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 4, 2024
@hyjorc1 hyjorc1 removed the merging-blocked Applied to prevent a change from being merged label Feb 5, 2024
@hyjorc1 hyjorc1 force-pushed the eng/JSC-refine-EXTR-pattern-detection-tests-for-b3-and-masm branch from 3959022 to ac3e0fa Compare February 5, 2024 17:45
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for ac3e0fa. Live statuses available at the PR page, #23820

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 5, 2024 •
edited
Loading

EWS run on previous version of this PR (hash ac3e0fa)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ⏳ 🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc   🧪 api-ios ⏳ 🧪 mac-wk2   🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv   🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
⏳ 🛠 tv-sim ✅ 🛠 jsc-armv7
⏳ 🛠 watch ⏳ 🧪 jsc-armv7-tests
⏳ 🛠 watch-sim

kmiller68
kmiller68 approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with nits.

Source/JavaScriptCore/assembler/testmasm.cpp Outdated Show resolved Hide resolved
Source/JavaScriptCore/b3/testb3_2.cpp

root->appendNewControlValue(
proc, Return, Origin(),
root->appendNew<Value>(proc, BitOr, Origin(), left, right));

auto code = compileProc(proc);
if (isARM64() && lowWidth > 0)
if (checkEmittedEXTR && isARM64() && 0 < lowWidth && lowWidth < 32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have checkEmittedEXTR? Shouldn't we always be checking for the instruction when we think we should have it?

Copy link
Contributor Author

@hyjorc1 hyjorc1 Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certain lowWidth values can trigger other optimizations in B3ReduceStrength.cpp, which can break this EXTR pattern. Then, checkEmittedEXTR is introduced since not every test case can emit EXTR.

Source/JavaScriptCore/b3/testb3_2.cpp

root->appendNewControlValue(
proc, Return, Origin(),
root->appendNew<Value>(proc, BitOr, Origin(), left, right));

auto code = compileProc(proc);
if (isARM64() && lowWidth > 0)
if (checkEmittedEXTR && isARM64() && 0 < lowWidth && lowWidth < 64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

@hyjorc1 hyjorc1 Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certain lowWidth values can trigger other optimizations in B3ReduceStrength.cpp, which can break this EXTR pattern. Then, checkEmittedEXTR is introduced since not every test case can emit EXTR.

@hyjorc1 hyjorc1 force-pushed the eng/JSC-refine-EXTR-pattern-detection-tests-for-b3-and-masm branch from ac3e0fa to 2bec550 Compare February 5, 2024 18:29
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Feb 5, 2024 •
edited
Loading

EWS run on current version of this PR (hash 2bec550)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ❌ 🧪 jsc-armv7-tests
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@hyjorc1 hyjorc1 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 6, 2024
@hyjorc1
[JSC] Refine B3 and masm tests for EXTR pattern detection and applica…
084df28 
…tion

https://bugs.webkit.org/show_bug.cgi?id=264984
rdar://118532295

Reviewed by Keith Miller.

Previously we introduced the pattern detection and application
of EXTR when B3 is lowering to AIR. This patch does two things:
1. Refine the corresponding B3 and masm tests for EXTR.
2. Add a sanity check and more comments for EXTR pattern detection and application in B3.

* Source/JavaScriptCore/assembler/testmasm.cpp:
(JSC::testExtractRegister32):
(JSC::testExtractRegister64):
* Source/JavaScriptCore/b3/B3LowerToAir.cpp:
* Source/JavaScriptCore/b3/testb3_2.cpp:
(testExtractRegister32):
(testExtractRegister64):

Canonical link: https://commits.webkit.org/274149@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-refine-EXTR-pattern-detection-tests-for-b3-and-masm branch from 2bec550 to 084df28 Compare February 6, 2024 17:24
@webkit-commit-queue
Copy link
Collaborator

Committed 274149@main (084df28): https://commits.webkit.org/274149@main

Reviewed commits have been landed. Closing PR #23820 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 084df28 into WebKit:main Feb 6, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmiller68 kmiller68 kmiller68 approved these changes

Assignees

@hyjorc1 hyjorc1

Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Milestone
No milestone
5 participants
@hyjorc1 @webkit-early-warning-system @webkit-commit-queue @kmiller68 @webkit-ews-buildbot