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

EA: perform analysis once for post-optimization IR, and remove IPO EA #51318

Merged
merged 2 commits into from
Sep 16, 2023

Conversation

aviatesk
Copy link
Member

Following the discussions and changes in #50805, we now consider post-inlining IR as IPO-valid. Revisiting EA, I've realized that running EA twice—once for computing IPO-valid escape cache and once for local optimization analysis—is redundant. This commit streamlines the EA process to perform the analysis just once on post-optimization IR, and caches that result. This change also removes all interprocedural EA code, which had significant overlap with inlining code.

@aviatesk aviatesk requested a review from Keno September 14, 2023 16:59
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM. But will this introduce any issues with convergence, or is that already addressed by #51092?

Also, do we need to be concerned with this that return_type_tfunc is marked EFFECTS_TOTAL and MethodResultPure, even though it would be a significant soundness issue to run code that contains that call via const-eval?

@aviatesk
Copy link
Member Author

But will this introduce any issues with convergence, or is that already addressed by #51092?

I don't foresee any convergence issues, as EA will be executed on each optimize-d frame and it won't be recursive. Although EA will cache and reuse results, it won't recurse down into a callee that hasn't been analyzed yet.

Also, do we need to be concerned with this that return_type_tfunc is marked EFFECTS_TOTAL and MethodResultPure, even though it would be a significant soundness issue to run code that contains that call via const-eval?

This seems like a separate concern? Considering the effects modeling of that function, I'm actually uncertain why it's marked EFFECTS_TOTAL. The return_type call does have an externally observable effect, because it creates an inference cache.

@aviatesk aviatesk force-pushed the avi/EA-simplify branch 2 times, most recently from d9a52d9 to c675a80 Compare September 15, 2023 06:23
@vtjnash
Copy link
Member

vtjnash commented Sep 15, 2023

EA will be executed on each optimize-d frame and it won't be recursive. Although EA will cache and reuse results, it won't recurse down into a callee that hasn't been analyzed yet.

I guess that is fine. It adds another reason that the computed results from inference are not consistent, but we already have fundamental reasons of computability that the computed effects cannot be accessed in a consistent-preserving way. (And I maintain my hope that someday we can expose Core.Compiler.return_effect optimization in the same way as Core.Compiler.return_type 😁)

@jpsamaroo jpsamaroo added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Sep 15, 2023
aviatesk and others added 2 commits September 16, 2023 15:05
Following the discussions and changes in #50805, we now consider
post-inlining IR as IPO-valid. Revisiting EA, I've realized that
running EA twice—once for computing IPO-valid escape cache and once for
local optimization analysis—is redundant. This commit streamlines the
EA process to perform the analysis just once on post-optimization IR,
and caches that result. This change also removes all interprocedural
EA code, which had significant overlap with inlining code.
@aviatesk aviatesk merged commit 6825abc into master Sep 16, 2023
@aviatesk aviatesk deleted the avi/EA-simplify branch September 16, 2023 08:29
NHDaly pushed a commit that referenced this pull request Sep 20, 2023
…A` (#51318)

Following the discussions and changes in #50805, we now consider
post-inlining IR as IPO-valid. Revisiting EA, I've realized that running
EA twice—once for computing IPO-valid escape cache and once for local
optimization analysis—is redundant. This commit streamlines the EA
process to perform the analysis just once on post-optimization IR, and
caches that result. This change also removes all interprocedural EA
code, which had significant overlap with inlining code.

---------

Co-authored-by: Julian Samaroo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants