Skip to content

Conversation

@tlively
Copy link
Member

@tlively tlively commented Dec 17, 2025

Fix comments and tests mentioning the experimental nominal type system
that existed before GC was standardized. As a drive-by, remove some
adjacent dead code in OptimizeInstructions that has not been relevant
since we updated If finalization to propage unreachability.

Fix comments and tests mentioning the experimental nominal type system
that existed before GC was standardized. As a drive-by, remove some
adjacent dead code in OptimizeInstructions that has not been relevant
since we updated If finalization to propage unreachability.
// (local.get $z)
// )
// )
assert(curr->ifTrue->type == curr->ifFalse->type);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not clear on why this assertion isn't needed? And why the if below it as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion isn't wrong, but it's not contributing much value after we remove the code below. The code below isn't necessary:

  • if newOuterType is unreachable, then both arms of the if or select must be unreachable, which means that curr->type is also unreachable and the branch will not be taken.
  • Otherwise newOuterType is not unreachable and neither arm is unreachable. In this case curr->type can only be unreachable if the if or select condition is unreachable. But since we're not changing the condition, the if or select will remain unreachable after optimizing and that unreachability will bubble up so that the curr->type continues to be unreachable after the optimization is applied, so there's no problem we need to avoid.

;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (nop)
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test was not previously testing what it claimed to test, which was the case where $types$1 is not the same as $type$2. After fixing the types to match the intent of the test, the optimization no longer applies.

@tlively tlively merged commit 58813d2 into main Dec 18, 2025
17 checks passed
@tlively tlively deleted the no-nominal branch December 18, 2025 16:38
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.

3 participants