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
----------------
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)
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
