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.

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

[JSC] CFA should clear abstract values first before reconstruction #14093

Conversation

hyjorc1
Copy link
Contributor

@hyjorc1 hyjorc1 commented May 19, 2023

3af657f

[JSC] CFA should clear abstract values first before reconstruction
https://bugs.webkit.org/show_bug.cgi?id=257044
rdar://109576467

Reviewed by Yusuke Suzuki.

Graph::packNodeIndices updates DFG nodes' indexes after packing,
which is usually performed in LivenessAnalysis phase. Since node
index is used for associating its abstract value, we usually need
to perform CFA subsequently to reconstruct abstract values for DFG graph.

However, the current implementation for CFA to reconstruct abstract
values is to reset their content according the new speculation without
cleaning first. This will bring us a problem that for some DFG nodes
e.g., EnumeratorNextUpdateIndexAndMode which shouldn't have speculation
type but might be updated with new node indexes after Graph::packNodeIndices.
With those updated node indexes, those DFG nodes might associate to typed
abstract values which is wrong. In this case, even CFA is performed subsequently,
those abstract values are still typed.

This patch fixes this issue by:
1. Clear abstract values after packing graph in debug build.
2. Do perform CFA in AI validation.
3. Clear abstract value for EnumeratorNextUpdateIndexAndMode in AI.

* JSTests/stress/regress-109263765.js: Added.
(foo):
* Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:
* Source/JavaScriptCore/dfg/DFGAtTailAbstractState.cpp:
(JSC::DFG::AtTailAbstractState::createOrClearValueForNode):
(JSC::DFG::AtTailAbstractState::createValueForNode): Deleted.
* Source/JavaScriptCore/dfg/DFGAtTailAbstractState.h:
* Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:
(JSC::DFG::CFAPhase::run):
* Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h:
(JSC::DFG::InPlaceAbstractState::createOrClearValueForNode):
(JSC::DFG::InPlaceAbstractState::createValueForNode): Deleted.
* Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.cpp:
(JSC::DFG::performGraphPackingAndLivenessAnalysis):
(JSC::DFG::performLivenessAnalysis): Deleted.
* Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.h:
* Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:
* Source/JavaScriptCore/dfg/DFGPlan.cpp:
(JSC::DFG::Plan::compileInThreadImpl):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::LowerDFGToB3):
(JSC::FTL::DFG::LowerDFGToB3::compileNode):

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

0a3f845

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2   πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ›  πŸ§ͺ jsc   πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv   πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  tv-sim βœ… πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch βœ… πŸ›  jsc-mips
βœ… πŸ›  watch-sim βœ… πŸ§ͺ jsc-mips-tests

@hyjorc1 hyjorc1 requested a review from a team as a code owner May 19, 2023 19:00
@hyjorc1 hyjorc1 self-assigned this May 19, 2023
@hyjorc1 hyjorc1 added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label May 19, 2023
@hyjorc1 hyjorc1 force-pushed the eng/JSC-CFA-should-clear-abstract-values-first-before-reconstruction branch from 934b1c7 to 2068a19 Compare May 19, 2023 22:28
Copy link
Member

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

@hyjorc1 hyjorc1 added merge-queue Applied to send a pull request to merge-queue and removed merge-queue Applied to send a pull request to merge-queue labels May 19, 2023
@hyjorc1 hyjorc1 force-pushed the eng/JSC-CFA-should-clear-abstract-values-first-before-reconstruction branch from 2068a19 to 0a3f845 Compare May 20, 2023 00:04
@hyjorc1 hyjorc1 added the merge-queue Applied to send a pull request to merge-queue label May 20, 2023
https://bugs.webkit.org/show_bug.cgi?id=257044
rdar://109576467

Reviewed by Yusuke Suzuki.

Graph::packNodeIndices updates DFG nodes' indexes after packing,
which is usually performed in LivenessAnalysis phase. Since node
index is used for associating its abstract value, we usually need
to perform CFA subsequently to reconstruct abstract values for DFG graph.

However, the current implementation for CFA to reconstruct abstract
values is to reset their content according the new speculation without
cleaning first. This will bring us a problem that for some DFG nodes
e.g., EnumeratorNextUpdateIndexAndMode which shouldn't have speculation
type but might be updated with new node indexes after Graph::packNodeIndices.
With those updated node indexes, those DFG nodes might associate to typed
abstract values which is wrong. In this case, even CFA is performed subsequently,
those abstract values are still typed.

This patch fixes this issue by:
1. Clear abstract values after packing graph in debug build.
2. Do perform CFA in AI validation.
3. Clear abstract value for EnumeratorNextUpdateIndexAndMode in AI.

* JSTests/stress/regress-109263765.js: Added.
(foo):
* Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:
* Source/JavaScriptCore/dfg/DFGAtTailAbstractState.cpp:
(JSC::DFG::AtTailAbstractState::createOrClearValueForNode):
(JSC::DFG::AtTailAbstractState::createValueForNode): Deleted.
* Source/JavaScriptCore/dfg/DFGAtTailAbstractState.h:
* Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:
(JSC::DFG::CFAPhase::run):
* Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.h:
(JSC::DFG::InPlaceAbstractState::createOrClearValueForNode):
(JSC::DFG::InPlaceAbstractState::createValueForNode): Deleted.
* Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.cpp:
(JSC::DFG::performGraphPackingAndLivenessAnalysis):
(JSC::DFG::performLivenessAnalysis): Deleted.
* Source/JavaScriptCore/dfg/DFGLivenessAnalysisPhase.h:
* Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:
* Source/JavaScriptCore/dfg/DFGPlan.cpp:
(JSC::DFG::Plan::compileInThreadImpl):
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::LowerDFGToB3):
(JSC::FTL::DFG::LowerDFGToB3::compileNode):

Canonical link: https://commits.webkit.org/264281@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/JSC-CFA-should-clear-abstract-values-first-before-reconstruction branch from 0a3f845 to 3af657f Compare May 20, 2023 01:25
@webkit-commit-queue
Copy link
Collaborator

Committed 264281@main (3af657f): https://commits.webkit.org/264281@main

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

@webkit-commit-queue webkit-commit-queue merged commit 3af657f into WebKit:main May 20, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 20, 2023
@csaavedra
Copy link
Member

Could you please fix the ambiguity in the assertion in line 58? It's causing some noisy warnings on Debug builds:

In file included from /app/webkit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-bfc896e1-3.cpp:3:
/app/webkit/Source/JavaScriptCore/dfg/DFGCFAPhase.cpp: In member function 'bool JSC::DFG::CFAPhase::run()':
/app/webkit/Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:58:111: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]

@hyjorc1
Copy link
Contributor Author

hyjorc1 commented May 24, 2023

Could you please fix the ambiguity in the assertion in line 58? It's causing some noisy warnings on Debug builds:

In file included from /app/webkit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-bfc896e1-3.cpp:3:
/app/webkit/Source/JavaScriptCore/dfg/DFGCFAPhase.cpp: In member function 'bool JSC::DFG::CFAPhase::run()':
/app/webkit/Source/JavaScriptCore/dfg/DFGCFAPhase.cpp:58:111: error: suggest parentheses around '&&' within '||' [-Werror=parentheses]

Thanks! Fixed in #14246.

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
5 participants