Skip to content

Conversation

gbaraldi
Copy link
Member

We were looking at partitions that could never be reached here, causing odd codegen behaviour.

Fixes some part of #42559

@gbaraldi gbaraldi requested review from Keno and vtjnash September 19, 2025 21:19
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Seems like it would fail to compute min world then? I'm confused by this

@Keno
Copy link
Member

Keno commented Sep 20, 2025

Yeah, this is probably the wrong fix. Either codegen needs to handle whatever partition structure is causing the problem, or inference can truncate the range in a particular scenario, but needs to be more precise than this.

@gbaraldi
Copy link
Member Author

It seems quite tricky for codegen to handle this if inference doesn't bound the world age to it's primary world. Because what is happening here is a global that has two partition, one that's visible in the current/world age of the method, and another that isn't. But those partitions have the same RTE (there's a comment there that guard should infer to Union{} but it doesn't), and with the same RTE we merge the partitions expanding the legal world age to the guard validity

@Keno
Copy link
Member

Keno commented Sep 20, 2025

You can extend the rte struct to flag this situation if codegen, can't handle it (although I could of course emit a world age check) but this fix is too aggressive.

this_rte = query(interp, leaf_binding, leaf_partition)
if @isdefined(rte)
if this_rte === rte
if this_rte === rte && !(is_some_guard(kind) is_some_guard(binding_kind(leaf_partition))) # Don't merge from no guard to guard
Copy link
Member

Choose a reason for hiding this comment

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

The same check should apply to backdated const, which needs to be treated uniformly with guard.

Copy link
Member Author

Choose a reason for hiding this comment

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

should is_some_guard be true for it?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's not a guard, but there should an is_backdated, which #59735 does add.

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