Skip to content

Conversation

gbaraldi
Copy link
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
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
@gbaraldi gbaraldi merged commit 7c89aba into master Feb 17, 2025
7 checks passed
@gbaraldi gbaraldi deleted the gb/late-lower-more-cases branch February 17, 2025 21:28
gbaraldi added a commit that referenced this pull request Feb 21, 2025
for (AllocaInst *Alloca : gc_allocas) {
llvm_dump(Alloca);
}
assert(false && "Expected single alloca");
Copy link
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
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