Skip to content

Make late_gc_lowering more robust#57380

Merged
gbaraldi merged 6 commits intomasterfrom
gb/late-lower-more-cases
Feb 17, 2025
Merged

Make late_gc_lowering more robust#57380
gbaraldi merged 6 commits intomasterfrom
gb/late-lower-more-cases

Conversation

@gbaraldi
Copy link
Copy Markdown
Member

There are cases where we optimize the SRet more than the pass expected so try and handle those. I'm tryin to get a test for this, this is separated from #52850 to make merging both easier

@gbaraldi gbaraldi requested a review from vchuravy February 12, 2025 21:13
@oscardssmith oscardssmith added the needs tests Unit tests are required for this change label Feb 12, 2025
Copy link
Copy Markdown
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but having a test would be great.

Maybe you can just manually write one? E.g. one with a phi node and one with a select?

@gbaraldi gbaraldi removed the needs tests Unit tests are required for this change label Feb 13, 2025
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
for (AllocaInst *Alloca : gc_allocas) {
llvm_dump(Alloca);
}
assert(false && "Expected single alloca");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason that this is an assert, somewhat annoying the message doesn't print on non assert builds.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Pushed a fix to your branch :)

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.

5 participants