[PATCH] D93656: Moving UniqueInternalLinkageNamesPass to the start of IR pipelines.

Wed Dec 23 12:57:09 PST 2020

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93656/new/

https://reviews.llvm.org/D93656