Page MenuHomePhabricator

Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.
ClosedPublic

Authored by hoy on Mon, Dec 21, 11:43 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aeubanks added inline comments.
llvm/include/llvm/Passes/PassBuilder.h
143

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

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 added a comment.Tue, Dec 22, 1:05 PM

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
143

Sounds good. Will move it into PipelineTuningOptions.

llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
26–29 ↗(On Diff #313187)

Sounds good, will remove it.

In D93656#2468834, @hoy 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 added inline comments.Tue, Dec 22, 1:39 PM
llvm/include/llvm/IR/DebugInfoMetadata.h
2056–2059 ↗(On Diff #313187)

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 ↗(On Diff #313187)

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 ↗(On Diff #313167)

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 added a comment.Tue, Dec 22, 10:31 PM
In D93656#2468834, @hoy 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.

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 updated this revision to Diff 313487.Tue, Dec 22, 10:34 PM

Separated PassBuilder changes.

In D93656#2469485, @hoy wrote:
In D93656#2468834, @hoy 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.

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 updated this revision to Diff 313488.Tue, Dec 22, 10:50 PM

Fixing IR test failure.

hoy added a comment.Tue, Dec 22, 11:21 PM

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

hoy edited the summary of this revision. (Show Details)Tue, Dec 22, 11:21 PM
hoy added a comment.EditedTue, Dec 22, 11:47 PM
In D93656#2469485, @hoy wrote:
In D93656#2468834, @hoy 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.

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 updated this revision to Diff 313566.Wed, Dec 23, 9:23 AM
hoy edited the summary of this revision. (Show Details)

Checking pipeline in clang test.

dblaikie added inline comments.Wed, Dec 23, 11:54 AM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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
2–3

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 added inline comments.Wed, Dec 23, 12:17 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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
2–3

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 added inline comments.Wed, Dec 23, 12:57 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 1:22 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 1:36 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 1:38 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 1:45 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 3:58 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 4:10 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 4:20 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Wed, Dec 23, 4:42 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 updated this revision to Diff 313630.Wed, Dec 23, 5:20 PM

Adding PTO checks in LLVM test.

aeubanks added inline comments.Wed, Dec 23, 6:50 PM
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
21

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

40

I don't think we need to check for Verifiers

45

why check ForceFunctionAttrsPass?

48

should this be run for O0?

hoy marked an inline comment as done.Wed, Dec 23, 8:35 PM
hoy added inline comments.
llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
21

It's not needed. Will remove.

40

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

45

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

48

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

hoy updated this revision to Diff 313650.Wed, Dec 23, 8:36 PM
hoy marked an inline comment as done.

Removing unnecessary test code.

aeubanks accepted this revision.Mon, Dec 28, 10:25 AM

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
19

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

This revision is now accepted and ready to land.Mon, Dec 28, 10:25 AM
dblaikie added inline comments.Tue, Dec 29, 2:15 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Tue, Dec 29, 2:22 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

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 added inline comments.Tue, Dec 29, 3:27 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
48–49 ↗(On Diff #313566)

Oh, great - thanks for splitting it up!

llvm/tools/opt/NewPMDriver.cpp
137–139

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

254–260

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 added inline comments.Tue, Dec 29, 5:17 PM
llvm/tools/opt/NewPMDriver.cpp
137–139

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?

254–260

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 updated this revision to Diff 314043.Tue, Dec 29, 5:52 PM

Removing the checks of VerifierAnalysis in test code.

aeubanks added inline comments.Wed, Dec 30, 7:30 AM
llvm/tools/opt/NewPMDriver.cpp
137–139

+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 added inline comments.Wed, Dec 30, 5:49 PM
llvm/tools/opt/NewPMDriver.cpp
137–139

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.

254–260

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 marked an inline comment as done.Thu, Dec 31, 11:44 AM
hoy added inline comments.
llvm/tools/opt/NewPMDriver.cpp
137–139

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

hoy updated this revision to Diff 314200.Thu, Dec 31, 11:44 AM
hoy marked an inline comment as done.

Adding a test for the new pseudo probe test.

dblaikie accepted this revision.Thu, Dec 31, 5:05 PM

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 added a comment.Sat, Jan 2, 1:34 PM

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 updated this revision to Diff 314257.Sat, Jan 2, 1:34 PM

Addressing comments from dblaikie.