Skip to content

Fix crash from [CGData] Global Merge Functions (#112671) #116241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 15, 2024
Merged

Fix crash from [CGData] Global Merge Functions (#112671) #116241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6254638
Fix crash from [CGData] Global Merge Functions (#112671)
kyulee-com Nov 14, 2024
0c8810e
Address comments from mikaelholmen
kyulee-com Nov 15, 2024
f96d717
Address comments from nocchijiang
kyulee-com Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalMergeFunctions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ class GlobalMergeFunc {

/// Global function merging pass for new pass manager.
struct GlobalMergeFuncPass : public PassInfoMixin<GlobalMergeFuncPass> {
const ModuleSummaryIndex *ImportSummary = nullptr;
GlobalMergeFuncPass() = default;
GlobalMergeFuncPass(const ModuleSummaryIndex *ImportSummary)
: ImportSummary(ImportSummary) {}
PreservedAnalyses run(Module &M, AnalysisManager<Module> &);
};

Expand Down
3 changes: 1 addition & 2 deletions llvm/lib/CodeGen/GlobalMergeFunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return Changed ? PreservedAnalyses::none() : PreservedAnalyses::all();
}
6 changes: 6 additions & 0 deletions llvm/test/CodeGen/Generic/cgdata-merge-crash.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
; This test checks if the global merge func pass should not build module summary.

; RUN: opt --passes=global-merge-func %s -o /dev/null

@0 = global { { ptr, i32, i32 } } { { ptr, i32, i32 } { ptr null, i32 19, i32 5 } }
@1 = global { { ptr, i32, i32 } } { { ptr, i32, i32 } { ptr null, i32 22, i32 5 } }
Loading