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

[Hongtao Yu via Phabricator via cfe-commits]

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


================
Comment at: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:1-2
+; RUN: opt -S -unique-internal-linkage-names < %s | FileCheck %s
+; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s
+
----------------
dblaikie wrote:
> Does this test test any changes that are made in the rest of the patch? Since the test is specifying the pass to test, I would've assumed this test would pass with or without any changes that might move that pass around in the pass order (or indeed add or remove it from the pass manager in general).
> 
> I'd expect this change to be tested in LLVM by a pass manager structure test, similar to the Clang test.
Right, the test here does not test the specific pass order set up by this change. It tests that the ordering change does not revert what's already there, i.e, the `UniqueInternalLinkageNamesPass` is still run and gives expected results.

Ideally the pipeline ordering should also be tested but that would require a LLVM switch setup to enable the pass and I'm not sure there's a need for that switch except for testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93656