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

[mono] Check if type is compatible right before emitting box #101509

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Apr 24, 2024

Currently, mono checks if type is compatible right after entering the world of handling box. However, there are many scenarios when the box could be optimized away. This PR moves the type compatible check to right before emitting box, when box is indeed needed.

This issue was uncovered by #101458

Copy link
Contributor

Tagging subscribers to this area: @lambdageek, @steveisok
See info in area-owners.md if you want to be subscribed.

@AaronRobinsonMSFT
Copy link
Member

@fanyang-mono Let's include these changes in the PR that is adding the tests. I will include them.

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Apr 24, 2024

@fanyang-mono Let's include these changes in the PR that is adding the tests. I will include them.

I suggest holding on doing that until CI qualification is clear here.

@fanyang-mono
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

All of the CI failures are known. All failures from runtime pipeline are already tracked. I opened two issues for the runtime-extra-platforms failures (#101549 and #101551)

@AaronRobinsonMSFT
Copy link
Member

All of the CI failures are known. All failures from runtime pipeline are already tracked. I opened two issues for the runtime-extra-platforms failures (#101549 and #101551)

@fanyang-mono I thought these changes were going to be included with the tests?

@fanyang-mono
Copy link
Member Author

All of the CI failures are known. All failures from runtime pipeline are already tracked. I opened two issues for the runtime-extra-platforms failures (#101549 and #101551)

@fanyang-mono I thought these changes were going to be included with the tests?

Do you mean the ByRefLike tests that you created in your PR, which uncovered this issue?

@AaronRobinsonMSFT
Copy link
Member

@fanyang-mono Yes. That was the point of the following comment - #101509 (comment). It is also why I included them in the PR. Having all this together is helpful when trying to understand PRs and fixes.

@fanyang-mono
Copy link
Member Author

@fanyang-mono Yes. That was the point of the following comment - #101509 (comment). It is also why I included them in the PR. Having all this together is helpful when trying to understand PRs and fixes.

Yeah that's a good point. Will do next time.

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants