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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49
 
+// LPIPELINE: Unique Internal Linkage Names
+// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass
 // PLAIN: @_ZL4glob = internal global
----------------
hoy wrote:
> dblaikie wrote:
> > hoy wrote:
> > > dblaikie wrote:
> > > > aeubanks wrote:
> > > > > dblaikie wrote:
> > > > > > hoy wrote:
> > > > > > > aeubanks wrote:
> > > > > > > > hoy wrote:
> > > > > > > > > aeubanks wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > hoy wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > > 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.
> > > > > > > > > > > > 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
> > > > > > > > > > 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)
> > > > > > > > > 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. 
> > > > > > > > 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).
> > > > > > > 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`.
> > > > > > 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).
> > > > > 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.
> > > > 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)).
> > > 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. 
> > 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.
> 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.
Oh, great - thanks for splitting it up!


================
Comment at: llvm/tools/opt/NewPMDriver.cpp:136-138
+static cl::opt<bool> PseudoProbeForProfiling(
+    "new-pm-pseudo-probe-for-profiling", cl::init(false), cl::Hidden,
+    cl::desc("Emit pseudo probes to enable PGO profile generation."));
----------------
I guess this should probably have some separate testing, if it's a separate flag/feature? (& flag+tests could be in a separate commit)


================
Comment at: llvm/tools/opt/NewPMDriver.cpp:253-258
     if (DebugInfoForProfiling)
       P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
                      true);
+    else if (PseudoProbeForProfiling)
+      P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
+                     false, true);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656