Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

[hoy]

Summary

UniqueInternalLinkageNamesPass is useful to CSSPGO, especially when pseudo probe is used. It solves naming conflict for static functions which otherwise will share a merged profile and likely have a profile quality issue with mismatched CFG checksums. Since the pseudo probe instrumentation happens very early in the pipeline, I'm moving UniqueInternalLinkageNamesPass right before it. This is being done only to the new pass manager.

[tmsriram]

I too was currently working on changing the raw linkage names, DW_AT_linkage_name, and was about to send out a patch along these lines :). Thanks for doing this! I will take a look at it.

[tmsriram]

https://reviews.llvm.org/D73307#1932131 rnk@ mentioned this : "At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen."

I just wanted to mention it here that this was anticipated and was missed in the original patch, my bad as I didnt think about DebugInfo change. However, I think it is pretty straightforward to change the linkage name so I would still keep the pass approach.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

27

Can we make it true by default? Atleast the DW_AT_linkage_name must reflect the new name by default IMO.

51

Do we need to check if it had a rawLinkageName before replacing it?

[hoy]

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

27

Sounds good.

51

Good point. rawLinkageName is missing for C programs. I think it might still be appropriate to assign a new linkage name in the case so as to differ from the original source function name. What do you think?

[hoy]

In D93656#2466689, @tmsriram wrote:

https://reviews.llvm.org/D73307#1932131 rnk@ mentioned this : "At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen."

I just wanted to mention it here that this was anticipated and was missed in the original patch, my bad as I didnt think about DebugInfo change. However, I think it is pretty straightforward to change the linkage name so I would still keep the pass approach.

Thanks for the information. Yeah it's pretty straightforward hook it up with debug info as an IR pass.

[tmsriram]

llvm/lib/IR/DebugInfoMetadata.cpp

881

Nit, Move the body to DebugInfoMetadata.h itself?

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

51

@dblaikie Adding the expert here. As far as I understand, the linkage name is the mangled name and this would capture this too correctly.

[hoy]

llvm/lib/IR/DebugInfoMetadata.cpp

881

Done.

[hoy]

Addressing feedbacks.

[dblaikie]

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))

In D93656#2466689, @tmsriram wrote:

https://reviews.llvm.org/D73307#1932131 rnk@ mentioned this : "At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen."

I just wanted to mention it here that this was anticipated and was missed in the original patch, my bad as I didnt think about DebugInfo change. However, I think it is pretty straightforward to change the linkage name so I would still keep the pass approach.

@rnk - any thoughts on this?

llvm/include/llvm/IR/DebugInfoMetadata.h

2056–2059

The need to add this API makes me a bit uncertain that it's the right direction - any chance you could find other examples of Function duplication in LLVM passes (maybe the partial inliner? Perhaps when it partially inlines an external function it has to clone the function so the external version remains identical?) and how they deal with debug info? Perhaps there's an existing API/mechanism to update the DISubprogram as you want without adding this.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

26–29

Do you have plans to use the flag in testing, etc? If not, I guess you might as well bake in the behavior you want if no one else is using this/there's no current use case for the flexibility?

48

What's this about/for? Should this be in an independent change?

[tmsriram]

llvm/include/llvm/IR/DebugInfoMetadata.h

2056–2059

This does not seem like a radical new API to me. We could use existing setOperand or replaceOperandWith here but need a public wrapper to expose it, just like getRawLinkageName. For example, DICompositeType is mutated like this with buildODRType.

[hoy]

llvm/include/llvm/IR/DebugInfoMetadata.h

2056–2059

Yeah, it would be nice to leverage an existing API like this but unfortunately I haven't found any.

Regarding passes that duplicate functions, I think the renaming done by UniqueInternalLinkageNamesPass is transparent to subsequent passes, since it is placed very early in the pipeline before any inlining or cloning passes.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

26–29

I'm not quite sure if updating debug names are required in all use cases. @tmsriram do you think we can just remove the flag?

48

This is about how the sample profile loader deals with renamed functions. The sample loader will use the original function name (i.e, by ignoring all suffixes appended) to retrieve a profile by default. With the attribute set here. the sample loader will only ignore certain suffixes not including the one (.__uniq.) appended here. Please see getCanonicalFnName in SampleProf.h for details. The reason that names are uniquefied here for AutoFDO is to have the sample loader differentiate same-named static functions, so the change is sort of related here.

I'll put more comments for that.

[tmsriram]

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

26–29

I vote for removing it.

[aeubanks]

llvm/include/llvm/Passes/PassBuilder.h

140

This seems better suited to be part of PipelineTuningOptions rather than directly in PassBuilder

[aeubanks]

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?

[hoy]

In 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?

llvm/include/llvm/Passes/PassBuilder.h

140

Sounds good. Will move it into PipelineTuningOptions.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

26–29

Sounds good, will remove it.

[aeubanks]

In D93656#2468834, @hoy wrote:

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

[dblaikie]

llvm/include/llvm/IR/DebugInfoMetadata.h

2056–2059

Not super radical, no - but more "we haven't had a need for this so far, I wonder how other similar use cases (if there are any) deal with this situation without this API addition"

Regarding passes that duplicate functions, I think the renaming done by UniqueInternalLinkageNamesPass is transparent to subsequent passes, since it is placed very early in the pipeline before any inlining or cloning passes.

Sorry, I wasn't meaning to ask "how does this pass interact with those other passes" but "how do those other passes deal with debug info, and can we learn anything from them/use similar techniques here that they use in their implementations"

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

48

Sounds like an independent patch, perhaps? (ie: Adding this attribute is relevant even if we weren't fixing the debug info name? And fixing the debug info name would be relevant even if we weren't adding this attribute) It'd be good for it to be in a separate patch to clarify the intent, test coverage, etc.

51

Best guess, yes.

Though the C case is interesting - it means you'll end up with C functions with the same DWARF 'name' but different linkage name. I don't know what that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, as much as they are with C++ overloading. I think there are some cases of C name mangling so it's probably supported/OK-ish with DWARF Consumers. Wouldn't hurt for you to take a look/see what happens in that case with a debugger like gdb/check other cases of C name mangling to see what DWARF they end up creating (with both GCC and Clang).

[hoy]

In D93656#2468841, @aeubanks wrote:

In D93656#2468834, @hoy wrote:

In 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, @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.

[hoy]

Separated PassBuilder changes.

[dblaikie]

In D93656#2469485, @hoy wrote:

In D93656#2468841, @aeubanks wrote:

In D93656#2468834, @hoy wrote:

In 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, @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

[hoy]

Fixing IR test failure.

[hoy]

The changes that rename debug linkage name has been separated as D93747. I'm moving the discussion there.

[hoy]

In D93656#2469504, @dblaikie wrote:

In D93656#2469485, @hoy wrote:

In D93656#2468841, @aeubanks wrote:

In D93656#2468834, @hoy wrote:

In 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, @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 -fdebug-pass-manager also works with the new pass manager.

[hoy]

Checking pipeline in clang test.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

Does this test validate the new behavior? (ie: does this test fail without the LLVM changes and pass with it) Not that it necessarily has to - since Clang isn't here to test the LLVM behavior - perhaps this test is sufficient in Clang to test that the code in BackendUtil works to enable this pass.

This could possibly be staged as independent commits - adding the LLVM functionality in one commit, which would be a no-op for Clang because it wouldn't be setting PTO.UniqueLinkageNames - then committing the Clang change that would remove the custom pass addition and set PTO.UniqueLinkageNames - and then it'd probably be reasonable to have this test be made a bit more explicit (testing the pass manager structure/order) to show that that Clang change had an effect: Moving the pass to the desired location in the pass pipeline.

llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

1–2 ↗

(On Diff #313566)

Does this test test any changes that are made in the rest of the patch? Since the test is specifying the pass to test, I would've assumed this test would pass with or without any changes that might move that pass around in the pass order (or indeed add or remove it from the pass manager in general).

I'd expect this change to be tested in LLVM by a pass manager structure test, similar to the Clang test.

[hoy]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

This is a good question. No, this test does not validate the pipeline change on the LLVM side, since Clang shouldn't have knowledge about how the pipelines are arranged in LLVM. As you pointed out, the test here is to test if the specific pass is run and gives expected results.

Thanks for the suggestion to break the Clang changes and LLVM changes apart which would make the testing more specific. The pipeline ordering could be tested with a LLVM test but that would require a LLVM switch setup for UniqueLinkageNames and I'm not sure there's a need for that switch except for testing.

llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

1–2 ↗

(On Diff #313566)

Right, the test here does not test the specific pass order set up by this change. It tests that the ordering change does not revert what's already there, i.e, the UniqueInternalLinkageNamesPass is still run and gives expected results.

Ideally the pipeline ordering should also be tested but that would require a LLVM switch setup to enable the pass and I'm not sure there's a need for that switch except for testing.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

No, this test does not validate the pipeline change on the LLVM side, since Clang shouldn't have knowledge about how the pipelines are arranged in LLVM.

"ish" - but Clang should have tests for changes to Clang, ideally. Usually they can simply be testing LLVM's IR output before it goes to LLVM for optimization/codegen - but for features that don't have this serialization boundary that makes testing and isolation clear/simple, it becomes a bit fuzzier.

In this case, there is a clang change - from adding the pass explicitly in Clang, to setting a parameter about how LLVM will add the pass, and it has an observable effect. One way to test this change while isolating the Clang test from further changes to the pipeline in LLVM, would be to test that the pass ends up somewhere in the LLVM-created part of the pass pipeline - the parts that you can't get to from the way the original pass addition was written in Clang. At least I assume that's the case/what motivated the change from adding it in Clang to adding it in LLVM?

eg: if LLVM always forms passes {x, y, z} and Clang is able to add passes before/after, say it always adds 'a' before and 'b' after, to make {a, x, y, z, b} - and this new pass u was previously added at the start to make {u, a, x, y, z, b} but now needs to go in {a, x, y, u, z, b} you could test that 'u' is after 'a' and before 'b', or between 'x' and 'z', etc. If there's some other more clear/simple/reliable marker of where the LLVM-created passes start/end in the structured dump, that'd be good to use as a landmark to make such a test more robust. If there's some meaningful pass that this pass always needs to go after - testing that might be OK, even if it's somewhat an implementation detail of LLVM - whatever's likely to make the test more legible and more reliable/resilient to unrelated changes would be good.

As you pointed out, the test here is to test if the specific pass is run and gives expected results.

If that's the case, this test could be committed standalone, before any of these other changes?

The pipeline ordering could be tested with a LLVM test but that would require a LLVM switch setup for UniqueLinkageNames and I'm not sure there's a need for that switch except for testing.

That's OK, the entire 'opt' tool and all its switches only exist for testing. eg: https://github.com/llvm/llvm-project/blob/master/llvm/tools/opt/NewPMDriver.cpp#L284

[aeubanks]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

The point of this change is that UniqueInternalLinkageNamesPass should run before SampleProfileProbePass. That must make a difference in the output of something like clang -emit-llvm -O1, right? Maybe we can add a new clang test that checks for that new change in IR, no need to check -fdebug-pass-manager. (I'm not familiar with the passes, correct me if I'm wrong)

[hoy]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

Maybe we can just keep the Clang test unchanged? What do you think? Since it's basically testing the command line switch -funique-internal-linkage-names works as expected, i.e, giving unique linkage names, it probably shouldn't care where the renaming happens exactly. Checking the pass order sounds a job to LLVM. I'll make the LLVM test do that.

The point of this change is that UniqueInternalLinkageNamesPass should run before SampleProfileProbePass.

Yeah, that's the point. We should probably make an LLVM test for it instead of a Clang test.

[aeubanks]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

An LLVM test sounds good, though you'll need a new cl::opt that the new option in PipelineTuningOptions defaults to (like other options in PipelineTuningOptions).

[hoy]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

Yeah, I was thinking about that too. I will also need a switch to trigger SampleProfileProbePass, like the exiting -new-pm-debug-info-for-profiling.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

I'm a bit confused by this thread of discussion.

Some fairly fundamental test architecture issues in the LLVM project: Code changes within a project should be tested within that project. Ideally they should test so as narrowly as possible so as not to produce failures due to unrelated changes.

This is usually fairly easy with anything in IR (test that Clang produces certain IR, test that optimization passes optimize that IR in certain ways, test that certain IR produces certain machine code, etc) - but harder with things that are represented only in API surface area (ie: there's no serialization of PipelineTuningOptions between Clang and LLVM - if there was, we could test that given a clang command line argument, the PTO has a certain property - then separately in LLVM we'd test that, given that PTO, a certain pass pipeline is constructed with the relevant features). In the absence of a serialization layer, we make a best effort in some way or another.

I think the best effort for a clang test for this clang change would be to dump the pass pipeline and ensure it has the property that's important - whatever property wasn't true before this change and is being made true by this change. Such as, as @aeubanks said, testing that UniqueInternalLinkageNamesPass comes before SampleProfileProbePass.

I think it is important that this is tested in Clang and separately that the functionality is tested in LLVM (by exposing the PTO parameter through opt, like other PTO parameters), probably in a similar manner (testing that given this PTO parameter, the pass pipeline has a certain shape). All of that separate from testing the pass itself does certain things when it is run (& that testing would be done in isolation - just running the specified pass).

[aeubanks]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

I'm not a fan of clang tests checking the output of -debug-pass-manager, it's checking implementation details that clang doesn't control. I'd prefer clang to just check that the pass ran somehow by examining the output IR given the clang cc1 flag. For example, maybe some function has __uniq in the name (maybe this test already checks something along these lines). Checking the exact PTO doesn't seem important. And for this change IMO clang doesn't need to test that some passes ran in some specific order, that's now an LLVM implementation detail.

The specifics of running UniqueInternalLinkageNamesPass before SampleProfileProbePass is now an LLVM thing, so an LLVM test should test that, whether it's checking -debug-pass-manager, or even better, checking the IR for certain properties.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

There's certainly no great answers here, imho. It's going to be tradeoffs for sure.

Clang tests executing the whole LLVM pipeline and checking the right answer out the otehr end means a lot more code under test - a lot more places that can have bugs that cause this test to fail that aren't just the one line in Clang the test is intended to test (it's not meant to test the LLVM functionality, that is tested in LLVM).

Clang does control some aspects of the pass pipeline - in this case moving the pass being added by clang explicitly, to asking LLVM to do it. Admittedly, yeah, no there's other aspects of implementation detail - Clang doesn't need to have any knowledge of specific pass names, or that this functionality is implemented by a pass.

All that said, as much as I don't find it great (tradeoffs for all answers here), yeah, I'm not going to veto an end-to-end test. I've certainly written them in the past when there really wasn't any other option (-fdebug-types-section, if I recall - MC flag with no observable effect until assembly is generated... no pass pipeline differences, etc (actually, maybe I just didn't test that at all, I forget which way I went - not ideal either way, to be sure)).

[hoy]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

Thanks for all the discussion and suggestions here. I think we all agree on making a LLVM test that checks the pipeline order as well as the output of that particular pass. Regarding the Clang test, since there's no for-sure answer, I'm inclined to leave it as is, i.e, without checking the exact PTO. This sounds a bit more robust to me since we'd like to isolate LLVM changes from Clang testing failures.

[hoy]

Adding PTO checks in LLVM test.

[aeubanks]

llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

20 ↗

(On Diff #313630)

is the debug info necessary for the test? it passes without it.

39 ↗

(On Diff #313630)

I don't think we need to check for Verifiers

44 ↗

(On Diff #313630)

why check ForceFunctionAttrsPass?

47 ↗

(On Diff #313630)

should this be run for O0?

[hoy]

llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

20 ↗

(On Diff #313630)

It's not needed. Will remove.

39 ↗

(On Diff #313630)

It shows up as the first pass of the O0 pipeline and I use it as a reference. Will remove.

44 ↗

(On Diff #313630)

Like VerifierPass above, I was using it as a reference.

47 ↗

(On Diff #313630)

There is no need for that as this point and running it under O0 is not tested.

[hoy]

Removing unnecessary test code.

[aeubanks]

LGTM with nit, but somebody else more familiar with this should LGTM to make sure this makes sense

llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

18 ↗

(On Diff #313650)

VerifierAnalysis also probably isn't necessary to check, same below

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

Seems like - if I'm understanding this correctly: "leave it as is" doesn't seem sufficient to me: Any test changes included with this patch should fail without it and pass with the code change (ie: demonstrate that the code change had a/the desired effect)

If this test change doesn't do that, it's both not suitable to include in this patch (since it's an unrelated change) and insufficient - because the production code change is untested.

[hoy]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

The changes in the Clang test have been undone, if you look at the latest iteration which is a pure llvm patch now. The code change in Clang will be in a separate patch.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names.cpp

49–50

Oh, great - thanks for splitting it up!

llvm/tools/opt/NewPMDriver.cpp

136–138 ↗

(On Diff #313650)

I guess this should probably have some separate testing, if it's a separate flag/feature? (& flag+tests could be in a separate commit)

253–258 ↗

(On Diff #313650)

Both of these might be more readably written as something like:

P.emplace();
P.PseudoProbeForProfiling = true;

& similar. (you can commit the change to DebugInfoForProfiling separately before/after this change to make it consistent with the new one)

But no big deal either way - while it makes these tidier it makes them a bit less consistent with the other three

[hoy]

llvm/tools/opt/NewPMDriver.cpp

136–138 ↗

(On Diff #313650)

I'm not sure there's a separate need for this switch except for being tested in unique-internal-linkage-names.ll. The point of this whole patch is to place the unique name pass before the pseudo probe pass and test it works. Hence it sounds appropriate to me to include all changes in one patch. What do you think?

253–258 ↗

(On Diff #313650)

That looks cleaner, but there are assertions in the constructor of PGOOptions which I would not like to bypass by setting the PseudoProbeForProfiling field separately.

[hoy]

Removing the checks of VerifierAnalysis in test code.

[aeubanks]

llvm/tools/opt/NewPMDriver.cpp

136–138 ↗

(On Diff #313650)

+1 to hoy's comment. I don't think there's a need to make patches strictly as incremental as possible if they're already small. (I would have been okay with keeping the Clang change here FWIW)

[dblaikie]

llvm/tools/opt/NewPMDriver.cpp

136–138 ↗

(On Diff #313650)

I understand I'm a bit of a stickler for some of this stuff - though the particular reason I brought it up here is that this adds two flags, but doesn't test them separately, only together. So it's not clear/tested as to which behaviors are provided by which flags.

Separating the flags would make it clear that each flag/functionality was tested fully.

Please add test coverage for each flag separately, optionally separate this into two patches to make it clearer how each piece of functionality is tested.

253–258 ↗

(On Diff #313650)

Ah, fair enough.

Though given the struct has publicly mutable members, these assertions aren't especially effective. Looks like they started being added here: https://reviews.llvm.org/D36040 - which I guess doesn't tell us much, but I was curious. Wonder if we could move the checks to somewhere more robust. (separate patch, in any case)

[hoy]

llvm/tools/opt/NewPMDriver.cpp

136–138 ↗

(On Diff #313650)

Sounds good, a separate test added for the pseudo probe flag.

[hoy]

Adding a test for the new pseudo probe test.

[dblaikie]

Looks good - test cases might benefit from some descriptive comments (explaining why the pseudo probe pass needs to be enabled to test the unique linkage name pass - I guess to check that it appears in just the right place in the pass pipeline?)

[hoy]

In D93656#2475856, @dblaikie wrote:

Looks good - test cases might benefit from some descriptive comments (explaining why the pseudo probe pass needs to be enabled to test the unique linkage name pass - I guess to check that it appears in just the right place in the pass pipeline?)

Comments added. Thanks for reviewing the patch.

[hoy]

Addressing comments from dblaikie.

clang/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
   // Only enable CGProfilePass when using integrated assembler, since
   // non-integrated assemblers don't recognize .cgprofile section.
   PTO.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
   PTO.Coroutines = LangOpts.Coroutines;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(CodeGenOpts.DebugPassManager);
   SI.registerCallbacks(PIC);
-  PassBuilder PB(CodeGenOpts.DebugPassManager, TM.get(), PTO, PGOOpt, &PIC);
+  PassBuilder PB(CodeGenOpts.DebugPassManager, TM.get(), PTO, PGOOpt, &PIC,
+                 CodeGenOpts.UniqueInternalLinkageNames);
 
   // Attempt to load pass plugins and register their callbacks with PB.
   for (auto &PluginFN : CodeGenOpts.PassPlugins) {
     auto PassPlugin = PassPlugin::Load(PluginFN);
     if (PassPlugin) {
       PassPlugin->registerPassBuilderCallbacks(PB);
     } else {
       Diags.Report(diag::err_fe_unable_to_load_plugin)
if (!CodeGenOpts.DisableLLVMPasses) {
     } else if (IsThinLTO) {
       MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
     } else if (IsLTO) {
       MPM = PB.buildLTOPreLinkDefaultPipeline(Level);
     } else {
       MPM = PB.buildPerModuleDefaultPipeline(Level);
     }
 
-    // Add UniqueInternalLinkageNames Pass which renames internal linkage
-    // symbols with unique names.
-    if (CodeGenOpts.UniqueInternalLinkageNames)
-      MPM.addPass(UniqueInternalLinkageNamesPass());
-
     if (!CodeGenOpts.MemoryProfileOutput.empty()) {
       MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
       MPM.addPass(ModuleMemProfilerPass());
     }
   }
 
   // FIXME: We still use the legacy pass manager to do code generation. We
   // create that pass manager here and use it as needed below.

clang/test/CodeGen/unique-internal-linkage-names.cpp

 // This test checks if internal linkage symbols get unique names with
 // -funique-internal-linkage-names option.
 // RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm -o - < %s | FileCheck %s --check-prefix=PLAIN
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUE
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O2 -S -emit-llvm -fexperimental-new-pass-manager -funique-internal-linkage-names -debug-info-kind=limited -mllvm -unqiue-debug-profile-names=true -o - < %s | FileCheck %s --check-prefix=UNIQUEDEBUG
 
 static int glob;
 static int foo() {
   return 0;
 }
 
 int (*bar())() {
   return foo;

 __attribute__((target("sse4.2"))) static int mver() {
   return 1;
 }
 
 int mver_call() {
   return mver();
 }
 
 // PLAIN: @_ZL4glob = internal global
 // PLAIN: @_ZZ8retAnonMvE5fGlob = internal global
 // PLAIN: @_ZN12_GLOBAL__N_16anon_mE = internal global
 // PLAIN: define internal i32 @_ZL3foov()
 // PLAIN: define internal i32 @_ZN12_GLOBAL__N_14getMEv
 // PLAIN: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // PLAIN: define internal i32 @_ZL4mverv()
 // PLAIN: define internal i32 @_ZL4mverv.sse4.2()
 // UNIQUE: @_ZL4glob.__uniq.{{[0-9a-f]+}} = internal global
 // UNIQUE: @_ZZ8retAnonMvE5fGlob.__uniq.{{[0-9a-f]+}} = internal global
 // UNIQUE: @_ZN12_GLOBAL__N_16anon_mE.__uniq.{{[0-9a-f]+}} = internal global
 // UNIQUE: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
 // UNIQUE: define internal i32 @_ZN12_GLOBAL__N_14getMEv.__uniq.{{[0-9a-f]+}}
 // UNIQUE: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // UNIQUE: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
 // UNIQUE: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
 // UNIQUEO1: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}()
 // UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
 // UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
+// UNIQUEDEBUG: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}() [[ATTR:#[0-9]+]] !dbg ![[#DBG:]]
+// UNIQUEDEBUG: attributes [[ATTR]] = { {{.*}} "sample-profile-suffix-elision-policy"="selected" {{.*}} }
+// UNIQUEDEBUG: ![[#DBG]] = distinct !DISubprogram(name: "foo", linkageName: "_ZL3foov.__uniq.{{[0-9a-f]+}}"

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
   }
   Metadata *getRawTemplateParams() const {
     return getNumOperands() > 9 ? getOperandAs<Metadata>(9) : nullptr;
   }
   Metadata *getRawThrownTypes() const {
     return getNumOperands() > 10 ? getOperandAs<Metadata>(10) : nullptr;
   }
 
+  void replaceRawLinkageName(MDString *LinkageName);
+
   /// Check if this subprogram describes the given function.
   ///
   /// FIXME: Should this be looking through bitcasts?
   bool describes(const Function *F) const;
 
   static bool classof(const Metadata *MD) {
     return MD->getMetadataID() == DISubprogramKind;
   }
 };
 

llvm/include/llvm/Passes/PassBuilder.h

 /// This class provides access to building LLVM's passes.
 ///
 /// Its members provide the baseline state available to passes during their
 /// construction. The \c PassRegistry.def file specifies how to construct all
 /// of the built-in passes, and those may reference these members during
 /// construction.
 class PassBuilder {
   bool DebugLogging;
+  bool UniqueLinkageNames;
   TargetMachine *TM;
   PipelineTuningOptions PTO;
   Optional<PGOOptions> PGOOpt;
   PassInstrumentationCallbacks *PIC;
 
 public:
   /// A struct to capture parsed pass pipeline names.
   ///
public:
     unsigned getSpeedupLevel() const { return SpeedLevel; }
 
     unsigned getSizeLevel() const { return SizeLevel; }
   };
 
   explicit PassBuilder(bool DebugLogging = false, TargetMachine *TM = nullptr,
                        PipelineTuningOptions PTO = PipelineTuningOptions(),
                        Optional<PGOOptions> PGOOpt = None,
-                       PassInstrumentationCallbacks *PIC = nullptr);
+                       PassInstrumentationCallbacks *PIC = nullptr,
+                       bool UniqueLinkageNames = false);
 
   /// Cross register the analysis managers through their proxies.
   ///
   /// This is an interface that can be used to cross register each
   /// AnalysisManager with all the others analysis managers.
   void crossRegisterProxies(LoopAnalysisManager &LAM,
                             FunctionAnalysisManager &FAM,
                             CGSCCAnalysisManager &CGAM,

llvm/lib/IR/DebugInfoMetadata.cpp

DEFINE_GETIMPL_STORE_N(
       Ops.size());
 }
 
 bool DISubprogram::describes(const Function *F) const {
   assert(F && "Invalid function");
   return F->getSubprogram() == this;
 }
 
+void DISubprogram::replaceRawLinkageName(MDString *LinkageName) {
+  replaceOperandWith(3, LinkageName);
+}
+
 DILexicalBlock *DILexicalBlock::getImpl(LLVMContext &Context, Metadata *Scope,
                                         Metadata *File, unsigned Line,
                                         unsigned Column, StorageType Storage,
                                         bool ShouldCreate) {
   // Fixup column.
   adjustColumn(Column);
 
   assert(Scope && "Expected scope");

llvm/lib/Passes/PassBuilder.cpp

 bool shouldPopulateClassToPassNames() {
   return !printBeforePasses().empty() || !printAfterPasses().empty();
 }
 
 } // namespace
 
 PassBuilder::PassBuilder(bool DebugLogging, TargetMachine *TM,
                          PipelineTuningOptions PTO, Optional<PGOOptions> PGOOpt,
-                         PassInstrumentationCallbacks *PIC)
-    : DebugLogging(DebugLogging), TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
+                         PassInstrumentationCallbacks *PIC,
+                         bool UniqueLinkageNames)
+    : DebugLogging(DebugLogging), UniqueLinkageNames(UniqueLinkageNames),
+      TM(TM), PTO(PTO), PGOOpt(PGOOpt), PIC(PIC) {
   if (TM)
     TM->registerPassBuilderCallbacks(*this, DebugLogging);
   if (PIC && shouldPopulateClassToPassNames()) {
 #define MODULE_PASS(NAME, CREATE_PASS)                                         \
   PIC->addClassToPassName(decltype(CREATE_PASS)::name(), NAME);
 #define MODULE_ANALYSIS(NAME, CREATE_PASS)                                     \
   PIC->addClassToPassName(decltype(CREATE_PASS)::name(), NAME);
 #define FUNCTION_PASS(NAME, CREATE_PASS)                                       \
PassBuilder::buildInlinerPipeline(OptimizationLevel Level, ThinLTOPhase Phase,
   return MIWP;
 }
 
 ModulePassManager
 PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
                                                ThinLTOPhase Phase) {
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (UniqueLinkageNames)
+    MPM.addPass(UniqueInternalLinkageNamesPass());
+
   // Place pseudo probe instrumentation as the first pass of the pipeline to
   // minimize the impact of optimization changes.
   if (PGOOpt && PGOOpt->PseudoProbeForProfiling &&
       Phase != ThinLTOPhase::PostLink)
     MPM.addPass(SampleProfileProbePass(TM));
 
   bool HasSampleProfile = PGOOpt && (PGOOpt->Action == PGOOptions::SampleUse);
 

 
 ModulePassManager PassBuilder::buildO0DefaultPipeline(OptimizationLevel Level,
                                                       bool LTOPreLink) {
   assert(Level == OptimizationLevel::O0 &&
          "buildO0DefaultPipeline should only be used with O0");
 
   ModulePassManager MPM(DebugLogging);
 
+  // Add UniqueInternalLinkageNames Pass which renames internal linkage
+  // symbols with unique names.
+  if (UniqueLinkageNames)
+    MPM.addPass(UniqueInternalLinkageNamesPass());
+
   if (PGOOpt && (PGOOpt->Action == PGOOptions::IRInstr ||
                  PGOOpt->Action == PGOOptions::IRUse))
     addPGOInstrPassesForO0(
         MPM,
         /* RunProfileGen */ (PGOOpt->Action == PGOOptions::IRInstr),
         /* IsCS */ false, PGOOpt->ProfileFile, PGOOpt->ProfileRemappingFile);
 
   for (auto &C : PipelineStartEPCallbacks)

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp

 //===- UniqueInternalLinkageNames.cpp - Unique Internal Linkage Sym Names -===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements unique naming of internal linkage symbols with option
 // -funique-internal-linkage-symbols.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
+static cl::opt<bool> UniqueDebugAndProfileNames(
+    "unqiue-debug-profile-names", cl::init(false), cl::Hidden,
+    cl::desc("Uniqueify debug and profile symbol Names"));
+
 static bool uniqueifyInternalLinkageNames(Module &M) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
   llvm::MD5::MD5Result R;
   Md5.final(R);
   SmallString<32> Str;
   llvm::MD5::stringifyResult(R, Str);
   // Prepend "__uniq" before the hash for tools like profilers to understand that
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto &F : M) {
     if (F.hasInternalLinkage()) {
       F.setName(F.getName() + ModuleNameHash);
+      if (UniqueDebugAndProfileNames) {
+        F.addFnAttr("sample-profile-suffix-elision-policy", "selected");
+        if (DISubprogram *SP = F.getSubprogram()) {
+          auto Name = MDB.createString(F.getName());
+          SP->replaceRawLinkageName(Name);
+        }
+      }
       Changed = true;
     }
   }
 
   // Append the module hash to all internal linkage globals.
   for (auto &GV : M.globals()) {
     if (GV.hasInternalLinkage()) {
       GV.setName(GV.getName() + ModuleNameHash);