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

Update LocalSubtyping for exact references #7355

Open
wants to merge 1 commit into
base: optimize-insts-exact
Choose a base branch
from

Conversation

tlively
Copy link
Member

@tlively tlively commented Mar 8, 2025

After calculating the best possible type for a local, LocalSubtyping
then checks to see whether the local can be non-nullable based on
whether all of its gets are dominated by sets. If it cannot be
non-nullable, the new type is adjusted to be nullable. This adjustment
did not previously preserve exactness, causing an assertion that the
optimization improves the type to fail. Fix the adjustment and add a
test.

After calculating the best possible type for a local, LocalSubtyping
then checks to see whether the local can be non-nullable based on
whether all of its gets are dominated by sets. If it cannot be
non-nullable, the new type is adjusted to be nullable. This adjustment
did not previously preserve exactness, causing an assertion that the
optimization improves the type to fail. Fix the adjustment and add a
test.
@tlively tlively requested a review from kripken March 8, 2025 03:45
@@ -152,7 +152,8 @@ struct LocalSubtyping : public WalkerPass<PostWalker<LocalSubtyping>> {
// Remove non-nullability if we disallow that in locals.
if (newType.isNonNullable()) {
if (cannotBeNonNullable.count(i)) {
newType = Type(newType.getHeapType(), Nullable);
newType =
Type(newType.getHeapType(), Nullable, newType.getExactness());
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it makes sense to add getAsNullable() or getWithNullability(nullability)? That would shorten such code, and be forward looking for when we add yet more things aside from shareability, nullability, exactness, etc....

Copy link
Member

Choose a reason for hiding this comment

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

Actually it could be getAs(nullability) which is nice and short.

@kripken
Copy link
Member

kripken commented Mar 10, 2025

I am ok with leaving such refactoring to later, but the more I think about it, while you are making these changes it would be good to do it, and not be any more work?

@tlively
Copy link
Member Author

tlively commented Mar 10, 2025

Yes, I've had the same thought. I was planning to leave such a refactoring to later, but you're right that it would make sense to update the use sites as I fix bugs incrementally. How about we land the current batch of PRs, and then I'll add the new API and update all these fixed use sites to use it. Subsequent bug fix PRs can then use the new API right away.

@kripken
Copy link
Member

kripken commented Mar 10, 2025

Sounds good.

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