Skip to content

Dataflow: Refactor access paths to split TypedContent into an explicit pair #12930

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

Merged
merged 26 commits into from
May 1, 2023

Conversation

aschackmull
Copy link
Contributor

In data flow we're tracking both a type and an access path. These two things are somewhat intertwined as we want to be able to reestablish the tracked type after a store-to-read step sequence.
We used to achieve this by merging the type into the front of the access paths resulting in a list of the form (typ1, content1) :: (typ2, content2) :: ... :: nil(typn). This refactor pulls the front type out of the access path, such that it becomes an explicit pair rather than being merged with Content. We still need the types nested inside the access path tails, so now, instead of being a list of TypedContents, access paths are a list of Contents where each nested tail is paired with a DataFlowType, i.e. (typ1, content1 :: (typ2, content2 :: ... :: (typn, nil)) ... )).
In the automaton view of access paths, we're switching from a graph with access-path nodes and type-content-pair-labelled edges to a graph where the nodes are type-access-path pairs and the edges are merely content-labelled.
This refactor is intended to pave the way for a future enhancement where we'll be able to update the tracked type independently from the access path to improve precision.

This PR is intended to be reviewed commit-by-commit. The commits progress by first duplicating the type information from the access paths where needed before replacing TypedContent with Content.

@aschackmull aschackmull force-pushed the dataflow/split-typedcontent branch from 6f21781 to 71ae090 Compare April 27, 2023 12:55
@aschackmull
Copy link
Contributor Author

DCA is looking good. The few additional results for Java appear to be caused by loss of precise type tracking stored in MapValueContent, and can be fixed by adding this to forceHighPrecision.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label May 1, 2023
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Looks really great; thanks a lot for splitting the work up into individual commits, that helped a lot. I only have one small question, but happy to merge as-is.

@@ -2138,6 +2163,8 @@ module Impl<FullStateConfigSig Config> {
PrevStage::revFlow(node, _) and result = TApproxFrontNil(node.getDataFlowType())
}

Typ getTyp(DataFlowType t) { result = t }
Copy link
Contributor

Choose a reason for hiding this comment

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

inline?

Copy link
Contributor

Choose a reason for hiding this comment

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

And same further down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect it to be extremely likely that the optimiser will in fact inline these, so no need to be explicit about that, I think.

@aschackmull aschackmull merged commit 6c8cb0d into github:main May 1, 2023
@aschackmull aschackmull deleted the dataflow/split-typedcontent branch May 1, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants