[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93656#2469504 <https://reviews.llvm.org/D93656#2469504>, @dblaikie wrote:

> In D93656#2469485 <https://reviews.llvm.org/D93656#2469485>, @hoy wrote:
>
>> In D93656#2468841 <https://reviews.llvm.org/D93656#2468841>, @aeubanks wrote:
>>
>>> In D93656#2468834 <https://reviews.llvm.org/D93656#2468834>, @hoy wrote:
>>>
>>>> In D93656#2468821 <https://reviews.llvm.org/D93656#2468821>, @aeubanks wrote:
>>>>
>>>>> Also it looks like this is doing 2 different things, the moving of things from Clang to LLVM's PassBuilder, and separately the change to the pass itself. Can these be separated?
>>>>
>>>> I'm not sure about a good way to separate them. There are Clang tests that may fail with removing the pass from clang while not adding it correspondingly in llvm. Adding the pass in llvm while not removing it from Clang may cause the pass to run twice which may also fail the Clang tests. What do you think?
>>>
>>> I mean keep that in one change, but separate out the change to llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and DebugInfoMetadata.h.
>>
>> I see, thanks for the clarification.
>>
>> In D93656#2468698 <https://reviews.llvm.org/D93656#2468698>, @dblaikie wrote:
>>
>>> This should have a LLVM test coverage for the LLVM changes. (I realize they're also tested by the Clang test, because there's no way to test Clang's pass manager creation short of testing the effect of running the pass manager (hmm - /maybe/ there's a way to dump the pass pipeline? In which case that's how Clang should be tested, just testing that it creates the right pipeline, not that that pipeline does any particular thing))
>>
>> Added an IR test. There is the llvm switch `-debug-pass=` that can dump the pass pipeline. I'm not aware of a clang switch that can do that.
>
> Seems some clang tests use something like that, eg:
>
>   clang/test/CodeGen/thinlto-debug-pm.c:// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O0 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fno-experimental-new-pass-manager -mllvm -debug-pass=Structure 2>&1 | FileCheck %s --check-prefix=O0-OLDPM

Thanks for pointing that out. It seems that only works with the legacy pass manager when `-emit-llvm` is specified. With a quick search in the clang regression tests it looks like the pipeline dumps are rarely checked there. Such checks are done quite a bit in llvm tests though. Does the current clang test look okay to you, or do you prefer checking the pipeline dump there? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656