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

optimizer: allow multiple inlining again #53911

Merged
merged 1 commit into from
Apr 1, 2024
Merged

optimizer: allow multiple inlining again #53911

merged 1 commit into from
Apr 1, 2024

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Apr 1, 2024

Before #52415 we could do multiple rounds of inlining on IR that had already been inlined, but this was no longer possible. By removing the assertion that was introduced in #52415, this commit makes it possible to do multi-inlining once more. Note that to fully solve this, though, we need to enhance ir_inline_linetable! so it can add new linetables to an inner linetable that's already been inlined. This commit notes that enhancement as something we need to do later, leaving it with a TODO comment.

@aviatesk aviatesk requested a review from vtjnash April 1, 2024 09:33
@aviatesk aviatesk changed the title allow multiple inlining again optimizer: allow multiple inlining again Apr 1, 2024
Before #52415 we could do multiple rounds of inlining on
IR that had already been inlined, but this was no longer possible. By
removing the assertion that was introduced in #52415,
this commit makes it possible to do multi-inlining once more. Note that
to fully solve this, though, we need to enhance `ir_inline_linetable!`
so it can add new linetables to an inner linetable that's already been
inlined. This commit notes that enhancement as something we need to do
later, leaving it with a TODO comment.
@Keno
Copy link
Member

Keno commented Apr 1, 2024

I'll merge this to fix downstream packages that are relying on this being possible. It shouldn't affect backtrace correctness on master, though I'm imagining it might in the multiple inlining case, so it'll be good to address the todo.

@Keno Keno merged commit 1fedcab into master Apr 1, 2024
5 of 7 checks passed
@Keno Keno deleted the avi/multi-inlining branch April 1, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants