hoy 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
----------------
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`.
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
