Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

Inliner: rewrite IDs on decorations fix#1059

Merged
eddyb merged 2 commits intoEmbarkStudios:mainfrom
SiebenCorgie:inliner-id-rewrite-decoratio-fix
May 26, 2023
Merged

Inliner: rewrite IDs on decorations fix#1059
eddyb merged 2 commits intoEmbarkStudios:mainfrom
SiebenCorgie:inliner-id-rewrite-decoratio-fix

Conversation

@SiebenCorgie
Copy link
Contributor

Currently the inliner only rewrites IDs in the 'inlined_callee_blocks'. Therefore 'OpDecorate %id' on any '%id' that is rewritten is lost in this pass.

This fix applies the rewrite rules to all decorations as well.

@SiebenCorgie SiebenCorgie requested a review from eddyb as a code owner May 5, 2023 09:55
Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

You need to duplicate annotations on inlining, not modifying the original - you should try testing with multiple call sites. (also, you should update the CHANGELOG)

// NOTE(siebencorgie): We don't care *what* decoration we rewrite atm. AFAIK there is no case where rewriting
// the decoration on inline wouldn't be valid.
for annotation_idx in 0..self.annotations.len() {
if self.annotations[annotation_idx].class.opcode == Op::Decorate {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is enough for your usecase but it should be possible to extend it to all decorate instructions (which I'll need for late zombie reporting).

@eddyb eddyb enabled auto-merge (rebase) May 26, 2023 16:19
auto-merge was automatically disabled May 26, 2023 16:28

Rebase failed

@eddyb eddyb enabled auto-merge (rebase) May 26, 2023 16:37
@eddyb
Copy link
Contributor

eddyb commented May 26, 2023

Sorry for the force push to your branch, had to rebase it to land it (I wonder if it's because of the merge commit you had - next time, use git pull --rebase instead of git pull, to avoid creating merge commits).

@eddyb eddyb merged commit 7a44fa1 into EmbarkStudios:main May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants