Page MenuHomePhabricator

Teach the analysis manager about inter-analysis dependencies.
Needs ReviewPublic

Authored by silvas on Aug 7 2016, 11:08 PM.

Details

Summary

This unifies analysis management for all IRUnitT's and adds dependency tracking.
The two are fairly of interrelated, since recursively invalidating transitive dependencies is difficult to do across separate analysis managers. Also, keeping track of the stack of analyses we are currently computing is difficult across separate analysis managers.

test/Other/analysis-manager/transitive-invalidation.ll shows a compelling test case.

I've used this in combination with http://reviews.llvm.org/D21921 to run the LTO pipeline on test-suite + SPEC cpu2006 with Asserts+ASan.
The two failures that remain are not related to this patch or approach (https://llvm.org/bugs/show_bug.cgi?id=28825 and https://llvm.org/bugs/show_bug.cgi?id=28888).

I've tried to retain the basic structure of the existing analysis manager code, but it still required touching quite a few lines and so the diff makes it look like it was totally rewritten.
A more incremental view of the patch can be seen here: https://github.com/llvm-mirror/llvm/compare/master...chisophugis:analysis-manager?expand=1

This patch requires making a small adjustment to every analysis and transform pass in tree (mostly, removing template parameters from AnalysisManager, and also removing use of the proxies), so there's a large number of mechanical changes.

This tracks downward invalidation using a dummy analysis called ParentIRUnitTrackingAnalysis.
By reusing the dependency tracking machinery, we get fine-grained invalidation of inner IRUnit's.
E.g. invaliding analyses on a single function doesn't invalidate loop analyses on all functions, which is a bug / fallout of the proxy approach (the InnerAnalysisManagerProxy calls "clear" on the entire LoopAnalysisManager).
TODO: test case for that

Diff Detail

Event Timeline

silvas updated this revision to Diff 67125.Aug 7 2016, 11:08 PM
silvas retitled this revision from to Teach the analysis manager about dependencies..
silvas updated this object.
silvas added a subscriber: llvm-commits.
silvas updated this revision to Diff 67126.Aug 7 2016, 11:31 PM

Add a test case of globals-aa invalidating AAManager to test/Other/analysis-manager/transitive-invalidation.ll.

silvas retitled this revision from Teach the analysis manager about dependencies. to Teach the analysis manager about inter-analysis dependencies..Aug 7 2016, 11:31 PM
davidxl edited edge metadata.Aug 8 2016, 9:47 AM

May I suggest a way to do reduce the diff?

  1. first do a nfc change with

typedef AnalysisManager<Function> FuncAnalysisManager;
....

and replace all AnalysisManager<function> with the typedef

  1. after that is in, this patch can be simplified with

typedef AnalysisManager FuncAnalysisManager;
...

  1. if 2) goes in, do another NFC to remove the typedef.
eraman added a subscriber: eraman.Aug 8 2016, 2:34 PM
silvas added a comment.Aug 8 2016, 3:08 PM

May I suggest a way to do reduce the diff?

  1. first do a nfc change with

    typedef AnalysisManager<Function> FuncAnalysisManager; ....

    and replace all AnalysisManager<function> with the typedef
  2. after that is in, this patch can be simplified with typedef AnalysisManager FuncAnalysisManager; ...
  3. if 2) goes in, do another NFC to remove the typedef.

Good suggestion. I'll try that (we already have the typedef's. We just don't use them consistently)

silvas updated this revision to Diff 67255.Aug 8 2016, 6:10 PM
silvas edited edge metadata.

Update based on David's suggestion.

This significantly reduces the diff from master. Hopefully it should be more tractable to review now.

Also a quick reminder that a more incremental view can be seen in: https://github.com/llvm-mirror/llvm/compare/master...chisophugis:analysis-manager

mehdi_amini added inline comments.Aug 8 2016, 11:30 PM
include/llvm/Analysis/AliasAnalysis.h
894

Wasn't the point of the refactoring with typedef to avoid this kind of diff?

silvas added inline comments.Aug 8 2016, 11:55 PM
include/llvm/Analysis/AliasAnalysis.h
894

I didn't eliminate all cases, but I did catch most. This case had a diff hunk that included a proxy change (below) so I kept it in this diff.

(I had to sort through hundreds of diff hunks so I was literally doing y/n to git checkout -p in order to get through them in a reasonable time)

mehdi_amini added inline comments.Aug 9 2016, 12:11 AM
include/llvm/Analysis/AliasAnalysis.h
894

Sure, I just wanted to be sure the patch was really ready before looking more into it.

Thanks for taking the time to reduce it!