Fix crash from [CGData] Global Merge Functions (#112671) #116241
Conversation
kyulee-com
force-pushed
the
fixsummary
branch
2 times, most recently
from
6010da4 to
c24e064
Compare
|
This fixes the crash I reported in #112671 (comment) I think it would be good to add a testcase though. There is a reproducer in #112671 (comment) so it's easy to make a testcase out of that. |
|
cc. @nocchijiang |
| @@ -0,0 +1,6 @@ | |||
| ; This test checks if the global merge func pass should not build module summary. | |||
There was a problem hiding this comment.
Just a nitpick: should we move the test file into llvm/test/CodeGen/AArch64/ or even llvm/test/CodeGen/Generic/ since this is not a ThinLTO/backend-specific test?
| @@ -666,7 +666,6 @@ bool GlobalMergeFuncPassWrapper::runOnModule(Module &M) { | |||
|
|
|||
| PreservedAnalyses GlobalMergeFuncPass::run(Module &M, | |||
| AnalysisManager<Module> &AM) { | |||
| ModuleSummaryIndex *Index = &(AM.getResult<ModuleSummaryIndexAnalysis>(M)); | |||
| bool Changed = GlobalMergeFunc(Index).run(M); | |||
| bool Changed = GlobalMergeFunc(ImportSummary).run(M); | |||
There was a problem hiding this comment.
If GlobalMergeFunc doesn't need ModuleSummaryIndex, why not just initialize without the parameter? Is there some weird side effect from some other thing that requires passing it in even though it's not used?
There was a problem hiding this comment.
This pass only uses this combined summary to determine if it's a LTO mode -- https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/GlobalMergeFunctions.cpp#L539-L542.
Currently, this pass is utilized in a pre-codegen pass that employs the legacy createGlobalMergeFuncPass(). As per the review comment, I have also incorporated an option for the new pass manager, GlobalMergeFuncPass(). If we intend to use this with the typical new PM, such as buildThinLTODefaultPipeline(), it is necessary to pass ImportSummary to the constructor.
kyulee-com
force-pushed
the
fixsummary
branch
3 times, most recently
from
35b8603 to
ac281be
Compare
kyulee-com
force-pushed
the
fixsummary
branch
from
ac281be to
f96d717
Compare
Module summary index is optional for this pass, and we shouldn't run it, but import it as necessary.