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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
