[mlir] proper "undefined" state in pointer analysis and correct handling of fresh state #1572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ftynse
force-pushed
the
ftynse/pessimize
branch
from
fb5ec7d
to
34fbff8
Compare
ftynse
force-pushed
the
ftynse/undefined
branch
from
09b9b31
to
d9b0cde
Compare
Historically, `AliasClassSet` has been using empty set as the bottom state of the join-semilattice. This made it impossible to differentiate between "not yet analyzed" and "known not to alias with anything" states. While not necessarily problematic for the naive alias analysis, this is crucial for points-to-pointer analysis that was forced to become unnecessarily conservative by treating "not yet analyzed" points-to state as "points to unknown" (top sate of the join-semilattice). Worse, some transfer functions weren't trivially monotonous because of that. Introduce a new "undefined" set to `AliasClassSet` and lattice. Update PointsToLattice to explicitly store the cases when a known alias class points to unknown alias class and interpret unknown (not yet listed) alias class as pointing to undefined alias class. Make all transfer functions monotonous by only processing known points-to entries. Unknown points-to classes can be treated by the client as potentially pointing to unknown, but so far this wasn't needed as, at the fixpoint state, all available classes should have been analyzed. Assertions are put in place in activity analysis to catch undefined classes. Additionally, make it more difficult to move the join-semilattice to a previous (<= current) value by exposing mostly the APIs that can only advance the lattice. This is not yet complete due to partially incorect handling of distinct identifers that get recreated every time in transfer functions.
Previously, dataflow initialization and transfer functions would
allocate a fresh alias class for allocation-like cases that are known
not to alias with anything, and do so _on every invocation_. This is
unsound in the general case as the class may keep changing, which could
affect convergence to fix point. Instead, add a cache that associates a
class with a value on the first invocation of the dataflow-related
functions and use the cached value on each subsequent invocation.
Fix a couple of issues unconvered due to the change:
- not recreate the common scope that function results may be pointing
to for each capturable operand;
- invert the condition of handling nocapture in function results;
- add a TODO for better handling of alias classes of function results
in presence of function-level attributes.
Specifically, handle the siutation where some alias classes are associateded with pointer operands that cannot be written into. In such a case, even if the results of the function may alias the operands, the alias classes of the operands are known not to point to anything that the function could have written. Additionally, don't include results marked as `noalias` into the common alias class of function results.
ftynse
force-pushed
the
ftynse/undefined
branch
from
d9b0cde
to
0e90f15
Compare
wsmoses
reviewed
Jan 18, 2024
wsmoses
approved these changes
Jan 18, 2024
MilesCranmer
pushed a commit
to MilesCranmer/Enzyme
that referenced
this pull request
Jul 24, 2024
* Fix getfield of const * fix * add test * fixup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Historically,
AliasClassSethas been using empty set as the bottomstate of the join-semilattice. This made it impossible to differentiate
between "not yet analyzed" and "known not to alias with anything"
states. While not necessarily problematic for the naive alias analysis,
this is crucial for points-to-pointer analysis that was forced to become
unnecessarily conservative by treating "not yet analyzed" points-to
state as "points to unknown" (top sate of the join-semilattice). Worse,
some transfer functions weren't trivially monotonous because of that.
Introduce a new "undefined" set to
AliasClassSetand lattice. UpdatePointsToLattice to explicitly store the cases when a known alias class
points to unknown alias class and interpret unknown (not yet listed)
alias class as pointing to undefined alias class. Make all transfer
functions monotonous by only processing known points-to entries. Unknown
points-to classes can be treated by the client as potentially pointing
to unknown, but so far this wasn't needed as, at the fixpoint state, all
available classes should have been analyzed. Assertions are put in place
in activity analysis to catch undefined classes.
Additionally, make it more difficult to move the join-semilattice to a
previous (<= current) value by exposing mostly the APIs that can only
advance the lattice. This is not yet complete due to partially incorrect
handling of distinct identifiers that get recreated every time in
transfer functions.