Skip to content

inference: invalidate stale slot wrapper types in ssavaluetypes#55551

Closed
aviatesk wants to merge 2 commits intomasterfrom
avi/issue55548
Closed

inference: invalidate stale slot wrapper types in ssavaluetypes#55551
aviatesk wants to merge 2 commits intomasterfrom
avi/issue55548

Conversation

@aviatesk
Copy link
Copy Markdown
Member

When updating a state of local variables from an assignment, all stale slot wrapper types must be invalidated. However, up until now, only those held in the local variable state were being invalidated. In fact, those held in ssavaluetypes also need to be invalidated, and this commit addresses that.

Currently all ssavaluetypes are iterated over with each assignment to a local variable, so this could potentially lead to performance issues. If so it might be necessary to perform invalidation more efficiently along with flow control.

@nanosoldier runbenchmarks("inference", vs=":master")

@aviatesk aviatesk requested a review from Keno August 21, 2024 13:10
@nanosoldier
Copy link
Copy Markdown
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@Keno
Copy link
Copy Markdown
Member

Keno commented Aug 21, 2024

I was concerned that making this change in the ssavaluetypes array messes with our convergence behavior, but I guess widening should be ok?

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Aug 22, 2024

I may have to write a fuller explanation later, but ssavaluetypes plays no role in reaching convergence and so there are no constraints on its behavior currently, but also no validity to changing an entry that is not the current pc. It is difficult for the existing algorithm to consider this change for correctness and potentially very costly (as seen by the 10x cost for just this incomplete change) to require the whole array to converge. There is some support (because of cycles) for rescheduling a basic block for analysis after changing an ssavalue result, but is generally expected for that situation to be rare (and not potentially any time a Conditional is created)

@aviatesk
Copy link
Copy Markdown
Member Author

As Jameson suggested on Slack, adding a guard on the side that consumes Conditional might be a better solution. This PR assumes that the slot assignment is always visited before any stale ssavaluetype is used, but that is not necessarily guaranteed in the algorithm.

@aviatesk
Copy link
Copy Markdown
Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Copy Markdown
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

When updating a state of local variables from an assignment, all stale
slot wrapper types must be invalidated. However, up until now, only
those held in the local variable state were being invalidated. In fact,
those held in `ssavaluetypes` also need to be invalidated, and this
commit addresses that.

Currently all `ssavaluetypes` are iterated over with each assignment to
a local variable, so this could potentially lead to performance issues.
If so it might be necessary to perform invalidation more efficiently
along with flow control.

- fixes #55548
@aviatesk
Copy link
Copy Markdown
Member Author

As Jameson suggested on Slack, adding a guard on the side that consumes Conditional might be a better solution.

I tried implementing 2224949 with this approach, but it ended up requiring significant changes to the Conditional implementation (and it's still WIP at implementing tmerge for Conditional). Honestly I'm unsure whether we would want to accept this level of complexity. A simpler approach of widening ssavaluetypes would solve the original issue without any performance regressions (the original performance regression was fixed by tracking SSAs where slot wrapper types are put into ssavaluetypes). What do you think about the approach we should take here?

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Sep 17, 2025

Replaced by #55601

@vtjnash vtjnash closed this Sep 17, 2025
@aviatesk aviatesk deleted the avi/issue55548 branch September 18, 2025 15:07
vtjnash added a commit that referenced this pull request Dec 30, 2025
This PR implements the "Path-Convergence Criterion" for SSA / ϕ-nodes as
part of type-inference.

The `VarState.ssadef` field corresponds to the "reaching definition" of
the slot in SSA form, which allows us to conveniently reason about the
identity of a `slot` across multiple program points. If the reaching def
is equal at two program points, then the slot contents are guaranteed to
be egal (i.e. `x₀ === x₁`)

Fixes #55548 (alternative to
#55551), by having `Conditional`
remember the reaching definition that it narrows.
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.

Conditional lattice element is semantically broken in the presence of SSAValues

4 participants