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