[PATCH] D115417: [LTO] Fix incomplete optimization remarks when PreOptModuleHook or PostInternalizeModuleHook is defined

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D115417#3186875 <https://reviews.llvm.org/D115417#3186875>, @Enna1 wrote:

> In D115417#3185707 <https://reviews.llvm.org/D115417#3185707>, @tejohnson wrote:
>
>> In D115417#3184794 <https://reviews.llvm.org/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 '
>>>   ...




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115417/new/

https://reviews.llvm.org/D115417