This is an archive of the discontinued LLVM Phabricator instance.

[LAA] Fix transitive analysis invalidation bug by implementing LoopAccessInfoManager::invalidate

Authored by bjope on Mar 16 2023, 2:48 AM.



The default invalidate method for analysis results is just looking
at the preserved state of the pass itself. It does not consider if
the analysis has an internal state that depend on other analyses.
Thus, we need to implement LoopAccessInfoManager::invalidate in order
to catch if LoopAccessAnalysis needs to be invalidated due to
transitive analyses such as AAManager is being invalidated. Otherwise
we might end up having references to an AAManager that is stale.


Diff Detail

Event Timeline

bjope created this revision.Mar 16 2023, 2:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 2:48 AM
bjope requested review of this revision.Mar 16 2023, 2:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 2:48 AM
nikic added inline comments.Mar 16 2023, 2:55 AM

What is the extra invalidate at the end of the pipeline for?

bjope added inline comments.Mar 16 2023, 6:16 AM

That was to see what happened when having an explicit invalidation of access-info. I.e. that the new invalidate function returns true in the early "// If not, give up now." exit, so that my patch wasn't lacking the check previously made by the default invalidate function.

Maybe nicer to extract that kind of test into a separate RUN-line?

aeubanks added inline comments.Mar 16 2023, 9:24 AM

yeah that seems better as a separate RUN

I also think that this can be simplified to just -passes=require<access-info>,invalidate<aa>


can we make this consistent by making the call order consistent?

bjope updated this revision to Diff 505871.Mar 16 2023, 10:35 AM
  1. Simplify return stmt in LoopAccessInfoManager::invalidate according to feedback.
  2. Use a separate RUN line in the test case to verify invalidata<access-info>.
  3. Reduce amount of CHECK:s in the test case. Don't think we need to verify what is going on with all other analyses. Better to make the CHECK:s focus on what is important for each sub-test.
bjope marked 2 inline comments as done.Mar 16 2023, 10:42 AM
bjope added inline comments.

I don't know why analyses tend to appear in different order (both when running and invalidating), e.g. when comparing an opt built using clang or with gcc. Maybe some set iteration that isn't guaranteeing iteration order.

I've updated this patch to use a bit less DAG checks. I don't think it was important to check everything.

But it is an interesting idea for the future to see if we can get it more consistent (lots of tests that are checking "Running analysis:" and "Invalidating analysis:" seem to use -DAG, which I think is a bit easy to forget when adding new tests).

aeubanks accepted this revision.Mar 16 2023, 10:47 AM
aeubanks added inline comments.

it's the order we call sub-analyses in LoopAccessAnalysis::run. the evaluation order of arguments is not necessarily consistent across all platforms. fetching them before the call to LoopAccessInfoManager should work (I've done that for other passes/analyses before)


I was wondering if the order of invalidation is guaranteed to be consistent, and I think it is as long as the order we fetch analyses is consistent because AnalysisResultListT is a std::list

This revision is now accepted and ready to land.Mar 16 2023, 10:47 AM
fhahn accepted this revision.Mar 16 2023, 10:57 AM
fhahn added a subscriber: fhahn.

Fix LGTM with comments by other people addressed.


Might be good to include a reference to the bug as well.


are those needed?

bjope marked an inline comment as done.Mar 16 2023, 11:18 AM
bjope added inline comments.

Ok. That sounds like a reasonable cause. I'll fix that for LoopAccessAnalysis::run.


Last I checked it was needed to get make the RUN on line 8 to crash. But since it crashes due to dangling references/pointers that might be a bit of a coincidence.

In order to just validate that the pass manager is doing invalidations, then I think the actual IR isn't important at all (i.e. we could remove more things and not only the attributes).

bjope added inline comments.Mar 17 2023, 1:36 AM

FWIW, I made a follow-up commit here:
It should make the analysis execution order more consistent, at least for analysis passes running other analyses.