[LTO] Fix incomplete optimization remarks for dead functions when PreOptModuleHook or PostInternalizeModuleHook is defined

[Enna1]

Summary

In https://reviews.llvm.org/rG20a895c4be01769a37dfffb3c6b513a7bc9b8d17, we introduce finalizeOptimizationRemarks() to make sure we flush the diagnostic remarks file in case the linker doesn't call the global destructors before exiting.
In https://reviews.llvm.org/D73597, we add optimization remarks for removed functions for debugging or for detecting dead code.
But there is a case, if PreOptModuleHook or PostInternalizeModuleHook is defined (e.g. --plugin-opt=emit-llvm is passed to linker), we do not call finalizeOptimizationRemarks(), therefore we will get an incomplete optimization remarks file.
This patch make sure we flush the diagnostic remarks file when PreOptModuleHook or PostInternalizeModuleHook is defined

[tejohnson]

This lgtm (looks like ThinLTO already has these calls for each of its hooks). Can you add a test?

[Enna1]

Thanks @tejohnson for looking into this !
I encounter this bug when I link a very large ELF file, and get an optimization remarks file(YAML format ) about 700MB and containing about 9000000 lines.
I try to write a small testcase to for this.
It seems that we create the optimization remarks file in lto::setupLLVMOptimizationRemarks() and get a ToolOutputFile, I did not find an option which controls the underlying buffer size used by ToolOutputFile.
Could you give me some hint or suggestions? Thanks!

[Enna1]

In https://reviews.llvm.org/D46376, we add support for optimization remarks to ThinLTO Backend, we call finalizeOptimizationRemarks() when PreOptModuleHook, PostPromoteModuleHook, PostInternalizeModuleHook, PostImportModuleHook is defined.

This patch call finalizeOptimizationRemarks() when PreOptModuleHook or PostInternalizeModuleHook is defined for Regular LTO Backend.

Cause llvm-lto2 does not have an option to define PreOptModuleHook or PreOptModuleHook for Regular LTO Backend, it's hard to write a testcase.

We can reproduce this bug using the following testcase:

$ clang++ -c -flto tu1.cpp -o tu1.o
$ clang++ -c -flto tu2.cpp -o tu2.o
$ clang++ -fuse-ld=lld -flto -foptimization-record-passes=lto tu1.o tu2.o -Wl,--opt-remarks-filename,opt.lto.yaml -Wl,--plugin-opt=emit-llvm

The content of tu1.cpp and tu2.cpp:

// tu1.cpp
int unused(int a);
int probably_inlined(int a);
int main(int argc, const char *argv[]) {
  return probably_inlined(argc);
}
// tu2.cpp
int unused(int a) {
  return a + 1;
}
int probably_inlined(int a) {
  return a + 2;
}

The content of opt.lto.yaml is empty or incomplete due to this bug.

If this patch is applied, the content of opt.lto.yaml will be:

--- !Passed
Pass:            lto
Name:            deadfunction
Function:        _Z6unusedi
Args:
  - Function:        _Z6unusedi
  - String:          ' not added to the combined module '
...

[tejohnson]

In D115417#3184794, @Enna1 wrote:

In https://reviews.llvm.org/D46376, we add support for optimization remarks to ThinLTO Backend, we call finalizeOptimizationRemarks() when PreOptModuleHook, PostPromoteModuleHook, PostInternalizeModuleHook, PostImportModuleHook is defined.

This patch call finalizeOptimizationRemarks() when PreOptModuleHook or PostInternalizeModuleHook is defined for Regular LTO Backend.

Cause llvm-lto2 does not have an option to define PreOptModuleHook or PreOptModuleHook for Regular LTO Backend, it's hard to write a testcase.

Ah, that would be a good thing to add support for testing in llvm-lto2, but not needed for this patch I think, see below.

We can reproduce this bug using the following testcase:

$ clang++ -c -flto tu1.cpp -o tu1.o
$ clang++ -c -flto tu2.cpp -o tu2.o
$ clang++ -fuse-ld=lld -flto -foptimization-record-passes=lto tu1.o tu2.o -Wl,--opt-remarks-filename,opt.lto.yaml -Wl,--plugin-opt=emit-llvm

You can just add a test via lld. I.e. see lld/test/ELF/lto/opt-remarks.ll. Can you add this case there? I believe the emit-llvm is what triggers the addition of the hooks, and that is not case being tested yet in that test.

The content of tu1.cpp and tu2.cpp:

// tu1.cpp
int unused(int a);
int probably_inlined(int a);
int main(int argc, const char *argv[]) {
  return probably_inlined(argc);
}
// tu2.cpp
int unused(int a) {
  return a + 1;
}
int probably_inlined(int a) {
  return a + 2;
}

The content of opt.lto.yaml is empty or incomplete due to this bug.

If this patch is applied, the content of opt.lto.yaml will be:

--- !Passed
Pass:            lto
Name:            deadfunction
Function:        _Z6unusedi
Args:
  - Function:        _Z6unusedi
  - String:          ' not added to the combined module '
...

[Enna1]

In D115417#3185707, @tejohnson wrote:

In D115417#3184794, @Enna1 wrote:

In https://reviews.llvm.org/D46376, we add support for optimization remarks to ThinLTO Backend, we call finalizeOptimizationRemarks() when PreOptModuleHook, PostPromoteModuleHook, PostInternalizeModuleHook, PostImportModuleHook is defined.

This patch call finalizeOptimizationRemarks() when PreOptModuleHook or PostInternalizeModuleHook is defined for Regular LTO Backend.

Cause llvm-lto2 does not have an option to define PreOptModuleHook or PreOptModuleHook for Regular LTO Backend, it's hard to write a testcase.

Ah, that would be a good thing to add support for testing in llvm-lto2, but not needed for this patch I think, see below.

I'm glad to send another patch to add an option that defines PreOptModuleHook or PreOptModuleHook for testing in llvm-lto2 :)

We can reproduce this bug using the following testcase:

$ clang++ -c -flto tu1.cpp -o tu1.o
$ clang++ -c -flto tu2.cpp -o tu2.o
$ clang++ -fuse-ld=lld -flto -foptimization-record-passes=lto tu1.o tu2.o -Wl,--opt-remarks-filename,opt.lto.yaml -Wl,--plugin-opt=emit-llvm

You can just add a test via lld. I.e. see lld/test/ELF/lto/opt-remarks.ll. Can you add this case there? I believe the emit-llvm is what triggers the addition of the hooks, and that is not case being tested yet in that test.

I'm sorry I forget to mention, I tried adding this bug case in lld/test/ELF/lto/opt-remarks.ll, but it not works.
Yes, the emit-llvm is what triggers the addition of the hooks, it also need Regular LTO detects some dead functions to test this bug case.
In lld/test/ELF/lto/opt-remarks.ll Regular LTO will not detected any dead function, and there is no optimization remarks for removed functions, so we can not simply add a case in lld/test/ELF/lto/opt-remarks.ll to test this bug case.

We can specify a symbol resolution using llvm-lto2, so we can make Regular LTO detect dead functions and emit optimization remarks for removed functions, when testing this bug via llvm-lto2.
But as I mentioned above, llvm-lto2 does not have an option that defines PreOptModuleHook or PreOptModuleHook, so llvm-lto2 can not trigger the addition of the hooks.

Thanks!

The content of tu1.cpp and tu2.cpp:

// tu1.cpp
int unused(int a);
int probably_inlined(int a);
int main(int argc, const char *argv[]) {
  return probably_inlined(argc);
}
// tu2.cpp
int unused(int a) {
  return a + 1;
}
int probably_inlined(int a) {
  return a + 2;
}

The content of opt.lto.yaml is empty or incomplete due to this bug.

If this patch is applied, the content of opt.lto.yaml will be:

--- !Passed
Pass:            lto
Name:            deadfunction
Function:        _Z6unusedi
Args:
  - Function:        _Z6unusedi
  - String:          ' not added to the combined module '
...

[tejohnson]

In D115417#3186875, @Enna1 wrote:

In D115417#3185707, @tejohnson wrote:

In D115417#3184794, @Enna1 wrote:

In https://reviews.llvm.org/D46376, we add support for optimization remarks to ThinLTO Backend, we call finalizeOptimizationRemarks() when PreOptModuleHook, PostPromoteModuleHook, PostInternalizeModuleHook, PostImportModuleHook is defined.

This patch call finalizeOptimizationRemarks() when PreOptModuleHook or PostInternalizeModuleHook is defined for Regular LTO Backend.

Cause llvm-lto2 does not have an option to define PreOptModuleHook or PreOptModuleHook for Regular LTO Backend, it's hard to write a testcase.

Ah, that would be a good thing to add support for testing in llvm-lto2, but not needed for this patch I think, see below.

I'm glad to send another patch to add an option that defines PreOptModuleHook or PreOptModuleHook for testing in llvm-lto2 :)

Cool, thanks

We can reproduce this bug using the following testcase:

$ clang++ -c -flto tu1.cpp -o tu1.o
$ clang++ -c -flto tu2.cpp -o tu2.o
$ clang++ -fuse-ld=lld -flto -foptimization-record-passes=lto tu1.o tu2.o -Wl,--opt-remarks-filename,opt.lto.yaml -Wl,--plugin-opt=emit-llvm

You can just add a test via lld. I.e. see lld/test/ELF/lto/opt-remarks.ll. Can you add this case there? I believe the emit-llvm is what triggers the addition of the hooks, and that is not case being tested yet in that test.

I'm sorry I forget to mention, I tried adding this bug case in lld/test/ELF/lto/opt-remarks.ll, but it not works.
Yes, the emit-llvm is what triggers the addition of the hooks, it also need Regular LTO detects some dead functions to test this bug case.
In lld/test/ELF/lto/opt-remarks.ll Regular LTO will not detected any dead function, and there is no optimization remarks for removed functions, so we can not simply add a case in lld/test/ELF/lto/opt-remarks.ll to test this bug case.

You could potentially just add a new test in that directory then, created with the example above.

We can specify a symbol resolution using llvm-lto2, so we can make Regular LTO detect dead functions and emit optimization remarks for removed functions, when testing this bug via llvm-lto2.
But as I mentioned above, llvm-lto2 does not have an option that defines PreOptModuleHook or PreOptModuleHook, so llvm-lto2 can not trigger the addition of the hooks.

Thanks!

The content of tu1.cpp and tu2.cpp:

// tu1.cpp
int unused(int a);
int probably_inlined(int a);
int main(int argc, const char *argv[]) {
  return probably_inlined(argc);
}
// tu2.cpp
int unused(int a) {
  return a + 1;
}
int probably_inlined(int a) {
  return a + 2;
}

The content of opt.lto.yaml is empty or incomplete due to this bug.

If this patch is applied, the content of opt.lto.yaml will be:

--- !Passed
Pass:            lto
Name:            deadfunction
Function:        _Z6unusedi
Args:
  - Function:        _Z6unusedi
  - String:          ' not added to the combined module '
...

[Enna1]

Add a test.

[tejohnson]

lgtm

[MaskRay]

lld/test/ELF/lto/Inputs/opt-remarks-incomplete.ll

1 ↗

(On Diff #393659)

Use split-file to avoid the auxiliary file.

lld/test/ELF/lto/opt-remarks-incomplete.ll

2

Add a file-level comment about the purpose of the test.

7

Use _start and remove -emain

8

[Enna1]

Address review comments.

[Enna1]

Can you commit this on my behalf cause I don't have commit access, thanks!

[MaskRay]

In D115417#3199208, @Enna1 wrote:

Can you commit this on my behalf cause I don't have commit access, thanks!

Testing.

lld/test/ELF/lto/opt-remarks-incomplete.ll

3

lld/test/ELF/lto/opt-remarks-incomplete.ll

+; REQUIRES: x86
+;; Ensure passing --plugin-opt=emit-llvm to lld, LTO should not emit
+;; incomplete optimization remarks for dead functions
+
+; RUN: split-file %s %t.dir
+; RUN: opt -module-summary %t.dir/main.ll -o %t1.o
+; RUN: opt -module-summary %t.dir/other.ll -o %t2.o
+
+; RUN: rm -f %t.yaml
+; RUN: ld.lld --plugin-opt=emit-llvm --opt-remarks-filename %t.yaml %t1.o %t2.o -o %t
+; RUN: FileCheck %s --check-prefix=REMARK < %t.yaml
+
+; REMARK:      Pass:            lto
+; REMARK-NEXT: Name:            deadfunction
+; REMARK-NEXT: DebugLoc:        { File: test.c, Line: 4, Column: 0 }
+; REMARK-NEXT: Function:        dead2
+; REMARK-NEXT: Args:
+; REMARK-NEXT:   - Function:        dead2
+; REMARK-NEXT:     DebugLoc:        { File: test.c, Line: 4, Column: 0 }
+; REMARK-NEXT:   - String:          ' not added to the combined module '
+
+#--- main.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @_start() {
+  call void @live1()
+  ret void
+}
+
+declare void @live1()
+
+define void @live2() {
+  ret void
+}
+
+define void @dead2() !dbg !7 {
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, emissionKind: LineTablesOnly, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 2}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"ThinLTO", i32 0}
+!7 = distinct !DISubprogram(name: "dead2", scope: !1, file: !1, line: 4, type: !8, isLocal: false, isDefinition: true, scopeLine: 4, isOptimized: false, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !2)
+
+#--- other.ll
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @live1() {
+  call void @live2()
+  ret void
+}
+
+declare void @live2()
+
+define void @dead1() {
+  call void @dead2()
+  ret void
+}
+
+declare void @dead2()

llvm/lib/LTO/LTO.cpp

Error LTO::runRegularLTO(AddStreamFn AddStream) {
   // If allowed, upgrade public vcall visibility metadata to linkage unit
   // visibility before whole program devirtualization in the optimizer.
   updateVCallVisibilityInModule(*RegularLTO.CombinedModule,
                                 Conf.HasWholeProgramVisibility,
                                 DynamicExportSymbols);
 
   if (Conf.PreOptModuleHook &&
       !Conf.PreOptModuleHook(0, *RegularLTO.CombinedModule))
-    return Error::success();
+    return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
 
   if (!Conf.CodeGenOnly) {
     for (const auto &R : GlobalResolutions) {
       if (!R.second.isPrevailingIRSymbol())
         continue;
       if (R.second.Partition != 0 &&
           R.second.Partition != GlobalResolution::External)
         continue;
for (const auto &R : GlobalResolutions) {
       if (EnableLTOInternalization && R.second.Partition == 0)
         GV->setLinkage(GlobalValue::InternalLinkage);
     }
 
     RegularLTO.CombinedModule->addModuleFlag(Module::Error, "LTOPostLink", 1);
 
     if (Conf.PostInternalizeModuleHook &&
         !Conf.PostInternalizeModuleHook(0, *RegularLTO.CombinedModule))
-      return Error::success();
+      return finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
   }
 
   if (!RegularLTO.EmptyCombinedModule || Conf.AlwaysEmitRegularLTOObj) {
     if (Error Err =
             backend(Conf, AddStream, RegularLTO.ParallelCodeGenParallelismLevel,
                     *RegularLTO.CombinedModule, ThinLTO.CombinedIndex))
       return Err;
   }