Skip to content

Conversation

@Constellation
Copy link
Member

@Constellation Constellation commented Sep 25, 2025

62300f8

[JSC] Immutable Load should look for the CSE target in dominators
https://bugs.webkit.org/show_bug.cgi?id=299504
rdar://161294228

Reviewed by Yijia Huang.

Previous one 300327@main worked for the same basic block's load,
but it didn't work for the load in dominators. This patch updates CSE
rules to make immutable load elimination work with dominators' load.

Like,

    BB#0
    @0: Load(@x, immutable)
    @1: CCall(...) # potentially clobber everything
    Branch ... #1, #2

    BB#1
    @2: CCall(...) # potentially clobber everything
    Jump #3

    BB#2
    @3: CCall(...) # potentially clobber everything
    Jump #3

    BB#3
    @4: Load(@x, immutable)
    ...

Then @4 should be replaced with Identity(@0) as dominator BB#0 is having
immutable load @0 matching to @4.

Tests: Source/JavaScriptCore/b3/testb3_1.cpp
       Source/JavaScriptCore/b3/testb3_8.cpp
* Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:
* Source/JavaScriptCore/b3/testb3.h:
* Source/JavaScriptCore/b3/testb3_1.cpp:
(run):
* Source/JavaScriptCore/b3/testb3_8.cpp:
(testLoadImmutableDominated):
(testLoadImmutableNonDominated):

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

7552a9a

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ✅ 🛠 ios-apple
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ✅ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 🧪 unsafe-merge ✅ 🛠 tv ✅ 🛠 mac-safer-cpp ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@Constellation Constellation self-assigned this Sep 25, 2025
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Sep 25, 2025
@Constellation Constellation force-pushed the eng/JSC-Immutable-Load-should-look-for-the-CSE-target-in-dominators branch from 8dee5a9 to 9ceb71a Compare September 25, 2025 21:37
@Constellation Constellation marked this pull request as ready for review September 25, 2025 21:37
@Constellation Constellation requested a review from a team as a code owner September 25, 2025 21:37
@Constellation Constellation force-pushed the eng/JSC-Immutable-Load-should-look-for-the-CSE-target-in-dominators branch from 9ceb71a to 7552a9a Compare September 25, 2025 22:05
Copy link
Contributor

@hyjorc1 hyjorc1 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

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Sep 26, 2025
https://bugs.webkit.org/show_bug.cgi?id=299504
rdar://161294228

Reviewed by Yijia Huang.

Previous one 300327@main worked for the same basic block's load,
but it didn't work for the load in dominators. This patch updates CSE
rules to make immutable load elimination work with dominators' load.

Like,

    BB#0
    @0: Load(@x, immutable)
    @1: CCall(...) # potentially clobber everything
    Branch ... WebKit#1, WebKit#2

    BB#1
    @2: CCall(...) # potentially clobber everything
    Jump WebKit#3

    BB#2
    @3: CCall(...) # potentially clobber everything
    Jump WebKit#3

    BB#3
    @4: Load(@x, immutable)
    ...

Then @4 should be replaced with Identity(@0) as dominator BB#0 is having
immutable load @0 matching to @4.

Tests: Source/JavaScriptCore/b3/testb3_1.cpp
       Source/JavaScriptCore/b3/testb3_8.cpp
* Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:
* Source/JavaScriptCore/b3/testb3.h:
* Source/JavaScriptCore/b3/testb3_1.cpp:
(run):
* Source/JavaScriptCore/b3/testb3_8.cpp:
(testLoadImmutableDominated):
(testLoadImmutableNonDominated):

Canonical link: https://commits.webkit.org/300562@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-Immutable-Load-should-look-for-the-CSE-target-in-dominators branch from 7552a9a to 62300f8 Compare September 26, 2025 07:25
@webkit-commit-queue
Copy link
Collaborator

Committed 300562@main (62300f8): https://commits.webkit.org/300562@main

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

@webkit-commit-queue webkit-commit-queue merged commit 62300f8 into WebKit:main Sep 26, 2025
@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 Sep 26, 2025
@Constellation Constellation deleted the eng/JSC-Immutable-Load-should-look-for-the-CSE-target-in-dominators branch September 26, 2025 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants