Page MenuHomePhabricator

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

Authored by Enna1 on Dec 8 2021, 6:51 PM.

Details

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

Diff Detail

Event Timeline

Enna1 created this revision.Dec 8 2021, 6:51 PM
Enna1 requested review of this revision.Dec 8 2021, 6:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 6:51 PM

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

Enna1 added a comment.Dec 9 2021, 2:02 AM

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 added a comment.Dec 10 2021, 1:15 AM

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 '
...

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 added a comment.EditedDec 10 2021, 6:12 PM

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 '
...

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 updated this revision to Diff 393659.Dec 10 2021, 8:35 PM
Enna1 retitled this revision from [LTO] Fix incomplete optimization remarks when PreOptModuleHook or PostInternalizeModuleHook is defined to [LTO] Fix incomplete optimization remarks for dead functions when PreOptModuleHook or PostInternalizeModuleHook is defined.

Add a test.

This revision is now accepted and ready to land.Dec 13 2021, 7:03 PM
MaskRay added inline comments.Dec 13 2021, 7:17 PM
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 updated this revision to Diff 394122.Dec 13 2021, 7:51 PM

Address review comments.

Enna1 marked 3 inline comments as done.Dec 14 2021, 3:44 AM

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

MaskRay accepted this revision.Dec 20 2021, 6:13 PM

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

Testing.

lld/test/ELF/lto/opt-remarks-incomplete.ll
4
This revision was landed with ongoing or failed builds.Dec 20 2021, 6:16 PM
This revision was automatically updated to reflect the committed changes.