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