[PM] Fix new LoopUnroll function pass by invalidating loop analysis
authorChandler Carruth <chandlerc@gmail.com>
Tue, 8 Aug 2017 02:24:20 +0000 (02:24 +0000)
committerChandler Carruth <chandlerc@gmail.com>
Tue, 8 Aug 2017 02:24:20 +0000 (02:24 +0000)
commit7c888dca46efae9b265da50509fc2b985d40eca3
tree162d8e89c34d1a1659d40726cfe3c59b0ff1354d
parent908ac3916d7e8d48dd69457e3a5c872b29ff17f9
[PM] Fix new LoopUnroll function pass by invalidating loop analysis
results when a loop is completely removed.

This is very hard to manifest as a visible bug. You need to arrange for
there to be a subsequent allocation of a 'Loop' object which gets the
exact same address as the one which the unroll deleted, and you need the
LoopAccessAnalysis results to be significant in the way that they're
stale. And you need a million other things to align.

But when it does, you get a deeply mysterious crash due to actually
finding a stale analysis result. This fixes the issue and tests for it
by directly checking we successfully invalidate things. I have not been
able to get *any* test case to reliably trigger this. Changes to LLVM
itself caused the only test case I ever had to cease to crash.

I've looked pretty extensively at less brittle ways of fixing this and
they are actually very, very hard to do. This is a somewhat strange and
unusual case as we have a pass which is deleting an IR unit, but is not
running within that IR unit's pass framework (which is what handles this
cleanly for the normal loop unroll). And where there isn't a definitive
way to clear *all* of the stale cache entries. And where the pass *is*
updating the core analysis that provides the IR units!

For example, we don't have any of these problems with Function analyses
because it is easy to clear out function analyses when the functions
themselves may have been deleted -- we clear an entire module's worth!
But that is too heavy of a hammer down here in the LoopAnalysisManager
layer.

A better long-term solution IMO is to require that AnalysisManager's
make their keys durable to this kind of thing. Specifically, when
caching an analysis for one IR unit that is conceptually "owned" by
a higher level IR unit, the AnalysisManager should incorporate this into
its data structures so that we can reliably clear these results without
having to teach each and every pass to do so manually as we do here. But
that is a change for another day as it will be a fairly invasive change
to the AnalysisManager infrastructure. Until then, this fortunately
seems to be quite rare.

llvm-svn: 310333
llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
llvm/test/Transforms/LoopUnroll/unroll-loop-invalidation.ll [new file with mode: 0644]