This is an archive of the discontinued LLVM Phabricator instance.

[AliasAnalysis] AAResults preserves AAManager.
ClosedPublic

Authored by asbirlea on Apr 19 2019, 12:22 PM.

Details

Diff Detail

Event Timeline

asbirlea created this revision.Apr 19 2019, 12:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2019, 12:22 PM

This looks right to me. Some suggestions on comment wording only.

lib/Analysis/AliasAnalysis.cpp
82–83

One nit about wording "Do not invalidate it here." -> "No need to check whether it has been preserved explicitly."

You should also update the comments on AAManager to explain this, and further add the (largely already in place) restriction that the registration functions must not be called once the AAManager is run.

I would also, merge this into the comment below as a single bit of prose. Could just continue after this comment with: "However, we still need to check if any of the dependencies end up being invalidated, and invalidate ourselves in that case."

You should also update the comments on AAManager to explain this, and further add the (largely already in place) restriction that the registration functions must not be called once the AAManager is run.

Could you please elaborate on this? How/where is this restriction enforced?

You should also update the comments on AAManager to explain this, and further add the (largely already in place) restriction that the registration functions must not be called once the AAManager is run.

Could you please elaborate on this? How/where is this restriction enforced?

It's not enforced at all, that's why we should document it.... We could add enforcement too, but it is a difficult mistake to make, and so I'm not worried about it. (Explained in person the nature of the "issue".)

asbirlea updated this revision to Diff 196103.Apr 22 2019, 11:15 AM
asbirlea marked an inline comment as done.

Address comment.

chandlerc accepted this revision.Apr 22 2019, 4:40 PM

LGTM

include/llvm/Analysis/AliasAnalysis.h
1100–1102 ↗(On Diff #196103)

This would be a bit easier for me to read as:

The result of this analysis are only invalidated if one of the
particular aggregated AA results end up being invalidated. This
removes the need to explicitly preserve the results of `AAManager`.
Note that analyses should no longer be registered once the
`AAManager` is run.
This revision is now accepted and ready to land.Apr 22 2019, 4:40 PM
This revision was automatically updated to reflect the committed changes.