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

Fix compiler crash in HeapToStack optimization pass #4067

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Fix compiler crash in HeapToStack optimization pass #4067

merged 1 commit into from
Mar 23, 2022

Conversation

SeanTAllen
Copy link
Member

This is a good one. One the March 22, 2022 development sync,
we collectively debugged issue #4059. We finally arrived at
the conclusion that our call graph usage was incorrect.

Originally during the call, Joe realized that while we were
using, we weren't doing it correctly. The initial assumption
was that the incorrect usage was innocous, however, it was
determined shortly thereafter that something about the incorrect
usage was in fact causing issue 4059.

This commit removes the offending code.

Gordon reported during the call that when the call graph code
was added as part the (now years ago) addition of LLVM 4 support
that the code was cribbed from the Dlang compiler and he posited,
probably not correctly.

Fixes #4059

@SeanTAllen SeanTAllen requested a review from a team March 23, 2022 11:48
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Mar 23, 2022
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 23, 2022
This is a good one. One the March 22, 2022 development sync,
we collectively debugged issue #4059. We finally arrived at
the conclusion that our call graph usage was incorrect.

Originally during the call, Joe realized that while we were
using, we weren't doing it correctly. The initial assumption
was that the incorrect usage was innocous, however, it was
determined shortly thereafter that something about the incorrect
usage was in fact causing issue 4059.

This commit removes the offending code.

Gordon reported during the call that when the call graph code
was added as part the (now years ago) addition of LLVM 4 support
that the code was cribbed from the Dlang compiler and he posited,
probably not correctly.

Fixes #4059
@jemc
Copy link
Member

jemc commented Mar 23, 2022

For posterity, I'll summarize my key takeaway from the call yesterday:

If we wanted to bring back this call graph analysis maintenance code, we'd want to first fix whatever the unknown bug in it is.

Then once it was working, in order to actually get the benefit of maintaining the call graph analysis, we'd want to tell LLVM that we maintained it by marking it as a "preserved analysis" in the analysisUsage function. Otherwise, LLVM is just going to discard the analysis and regenerate it anyway.

@SeanTAllen SeanTAllen merged commit b80287c into main Mar 23, 2022
@SeanTAllen SeanTAllen deleted the 4059 branch March 23, 2022 20:02
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Mar 23, 2022
github-actions bot pushed a commit that referenced this pull request Mar 23, 2022
github-actions bot pushed a commit that referenced this pull request Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler crash in HeapToStack optimization
3 participants