Skip to content

Commit

Permalink
lgc: Workaround crash issue with stop-after option
Browse files Browse the repository at this point in the history
The issue was first observed by running:
`amdllpc -gfxip=10.3 -stop-after=lgc-patch-resource-collect llpc/test/shaderdb/general/PipelineGsTess_TestInOutPacking.pipe -o -`.

Based on my limited experiment, there are three key factors to trigger
this issue:
1.) There is a `stop-after` option
2.) the stopped after name points to a pass before optimization
3.) The input IR to lgc stage have a loop

Using `lgc` command can also reproduce the issue by generating the
input `.lgc` through amdllpc first.

Based on my observation, the issue happens mainly because the
`InnerAnalysisManagerProxy::Result::~Result()` try to clear()
the things in the corresponding InnerAnalysisManager(aka `InnerAM`).

For our specific case, the LoopAnalysisManager (in derived class) was
destructed before the ModuleAnalysisManager (in base class).  After
the LoopAnalysisManager was destroyed, there is still an
`AnalysisResultModel` object in `AnalysisResults` of
FunctionAnalysisManager holding pointer to the LoopAnalysisManger
(stale now!), and which was transitively in the ModuleAnalysisManager.
When ModuleAnalysisManager is destroyed, it will try to clear() the
`AnalysisResults` in LoopAnalysisManager because of the outdated
`InnerAM` pointer to the LoopAnalysisManager, but the
LoopAnalysisManager itself and all its memembers are freed. The
problem should have more deeper reason, but it is hard for me to
figure out why it was designed like this.

I also did some interesting experiment and it shows the problem is not
specific to LoopAnalysisManager, but also applies to
FunctionAnalysisManager. If we change the order of FunctionAnalysisManger
and ModuleAnalysisManager (that is change their destruction order) in
lgc::PassManager, we can also see the crash running:
`lgc -stop-after=lgc-patch-buffer-op -o - lgc/test/ScalarizeInputWithDynamicIndexUser.lgc`.

Another experiment I did to change the declaration order of the
anlysis managers in llvm/tools/opt/NewPMDriver.cpp. Crash can also be
observed running `check-llvm`.

I think there should be more robust solution to fix the issue properly.
But I am completely unsure what it would be look like. So I post this
change to unblock the usage of `stop-after` on certain early patch
passes for our lit-testing.

The `CGSCCAnalysisManager` order change is not mandatory for the issue I
see, but I just change it to be consistent.
  • Loading branch information
ruiling committed Oct 23, 2023
1 parent 85c9345 commit a0ffb64
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 2 deletions.
4 changes: 4 additions & 0 deletions lgc/interface/lgc/PassManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ class PassManager : public llvm::ModulePassManager {
virtual llvm::PassInstrumentationCallbacks &getInstrumentationCallbacks() = 0;

protected:
// NOTE: The analysis managers are intentionally placed in inner->outer order so that the outmost analysis is
// destructed first, this help workaround crash when stop-after is used.
llvm::LoopAnalysisManager m_loopAnalysisManager;
llvm::FunctionAnalysisManager m_functionAnalysisManager;
llvm::CGSCCAnalysisManager m_cgsccAnalysisManager;
llvm::ModuleAnalysisManager m_moduleAnalysisManager;
};

Expand Down
2 changes: 0 additions & 2 deletions lgc/util/PassManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,6 @@ class PassManagerImpl final : public lgc::PassManager {

// -----------------------------------------------------------------------------------------------------------------

LoopAnalysisManager m_loopAnalysisManager; // Loop analysis manager used when running the passes.
CGSCCAnalysisManager m_cgsccAnalysisManager; // CGSCC analysis manager used when running the passes.
PassInstrumentationCallbacks m_instrumentationCallbacks; // Instrumentation callbacks ran when running the passes.
StandardInstrumentations m_instrumentationStandard; // LLVM's Standard instrumentations
unsigned *m_passIndex = nullptr; // Pass Index.
Expand Down

0 comments on commit a0ffb64

Please sign in to comment.