Switching Clang UniqueInternalLinkageNamesPass scheduling to using the LLVM one with newpm.

[hoy]

Summary

As a follow-up to D93656, I'm switching the Clang UniqueInternalLinkageNamesPass scheduling to using the LLVM one with newpm.

Test Plan:

[dblaikie]

Please add a clang test for this.

[aeubanks]

I don't think a test is necessary for an NFC change.

[hoy]

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

[dblaikie]

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

[dblaikie]

In D94019#2478046, @aeubanks wrote:

I don't think a test is necessary for an NFC change.

My understanding is that this change does have a functional change. It's not a refactoring change but changes the resulting output of Clang/LLVM, right?

[hoy]

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

[dblaikie]

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

[hoy]

In D94019#2478253, @dblaikie wrote:

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

The change on the LLVM side is observable on the IR level when pseudo probe is enabled. I think that LLVM change is transparent to Clang. In other words, Clang doesn't need to know that the LLVM change is to favor pseudo probe. It just needs to know that LLVM already has a unique name pass scheduled and itself should not schedule the same pass again. What do you think?

[dblaikie]

In D94019#2478277, @hoy wrote:

In D94019#2478253, @dblaikie wrote:

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

The change on the LLVM side is observable on the IR level when pseudo probe is enabled. I think that LLVM change is transparent to Clang. In other words, Clang doesn't need to know that the LLVM change is to favor pseudo probe.

Clang's overall, user-facing behavior changed (otherwise why make this change, right?) due to a change to Clang's source code, so it should be tested to demonstrate that the source change had the desired effect and doesn't regress. These particular changes are awkward because things like CodeGenOpts aren't serialized, unlike LLVM IR, but that doesn't mean they shouldn't be tested - it means the testing isn't as elegant as it is for serialized IR.

[hoy]

In D94019#2478284, @dblaikie wrote:

In D94019#2478277, @hoy wrote:

In D94019#2478253, @dblaikie wrote:

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

The change on the LLVM side is observable on the IR level when pseudo probe is enabled. I think that LLVM change is transparent to Clang. In other words, Clang doesn't need to know that the LLVM change is to favor pseudo probe.

Clang's overall, user-facing behavior changed (otherwise why make this change, right?) due to a change to Clang's source code, so it should be tested to demonstrate that the source change had the desired effect and doesn't regress. These particular changes are awkward because things like CodeGenOpts aren't serialized, unlike LLVM IR, but that doesn't mean they shouldn't be tested - it means the testing isn't as elegant as it is for serialized IR.

Yes, the user-facing behavior changed and it is tested by an LLVM test. I think that's transparent to clang and Clang doesn't know why that change is made in LLVM. A possible clang test could be to enable pseudo probe and check the output of pseudo probe metadata where unique linkage names are encoded. But we don't even has such a test in the LLVM part either.

[dblaikie]

In D94019#2478308, @hoy wrote:

In D94019#2478284, @dblaikie wrote:

In D94019#2478277, @hoy wrote:

In D94019#2478253, @dblaikie wrote:

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

The change on the LLVM side is observable on the IR level when pseudo probe is enabled. I think that LLVM change is transparent to Clang. In other words, Clang doesn't need to know that the LLVM change is to favor pseudo probe.

Clang's overall, user-facing behavior changed (otherwise why make this change, right?) due to a change to Clang's source code, so it should be tested to demonstrate that the source change had the desired effect and doesn't regress. These particular changes are awkward because things like CodeGenOpts aren't serialized, unlike LLVM IR, but that doesn't mean they shouldn't be tested - it means the testing isn't as elegant as it is for serialized IR.

Yes, the user-facing behavior changed and it is tested by an LLVM test. I think that's transparent to clang and Clang doesn't know why that change is made in LLVM. A possible clang test could be to enable pseudo probe and check the output of pseudo probe metadata where unique linkage names are encoded. But we don't even has such a test in the LLVM part either.

If this didn't involve a code change in Clang, I'd generally agree with you - but this is a code change in Clang required/intended to elicit a change in behavior, and should be tested to demonstrate that change in behavior.

A possible clang test could be to enable pseudo probe and check the output of pseudo probe metadata where unique linkage names are encoded. But we don't even has such a test in the LLVM part either.

Yes, I think that would be one valid option - or a test that validates the change in the pass order, like the LLVM test. Either would be acceptable to me - I'd personally lean more towards the pass order test, like the LLVM one, and @aeubanks has expressed a preference towards and end-to-end type test, I believe.

[aeubanks]

Oh sorry, yeah this isn't NFC.

But I still don't think this needs a new test. We're going from a custom Clang implementation of adding passes to something more generic that's also more tested within LLVM.
So IMO we just need an end to end test, which we already have in unique-internal-linkage-names.cpp. The details are tested on the LLVM side.

[dblaikie]

In D94019#2478378, @aeubanks wrote:

Oh sorry, yeah this isn't NFC.

But I still don't think this needs a new test. We're going from a custom Clang implementation of adding passes to something more generic that's also more tested within LLVM.
So IMO we just need an end to end test, which we already have in unique-internal-linkage-names.cpp. The details are tested on the LLVM side.

Then could the end to end test be tightened up to demonstrate how this patch/the change in pass order has produced new/different/desired behavior?

[aeubanks]

In D94019#2478423, @dblaikie wrote:

In D94019#2478378, @aeubanks wrote:

Oh sorry, yeah this isn't NFC.

But I still don't think this needs a new test. We're going from a custom Clang implementation of adding passes to something more generic that's also more tested within LLVM.
So IMO we just need an end to end test, which we already have in unique-internal-linkage-names.cpp. The details are tested on the LLVM side.

Then could the end to end test be tightened up to demonstrate how this patch/the change in pass order has produced new/different/desired behavior?

It could be, but again I don't see the point. Rather than thinking about testing within individual changes, I think thinking about the overall state of testing regarding UniqueInternalLinkageNames after this change is better.
The only Clang test we need is to make sure PTO.UniqueLinkageNames is properly flipped, since that is the only thing Clang does regarding UniqueInternalLinkageNames . We have that Clang test in unique-internal-linkage-names.cpp (we could have a test that checks the PTO if it were serialized as you've said, but unique-internal-linkage-names.cpp is fine). It would fail if we didn't do PTO.UniqueLinkageNames = CodeGenOpts.UniqueInternalLinkageNames;.
Testing exactly what PTO.UniqueLinkageNames does is better done in LLVM. And that has already been done.

[dblaikie]

In D94019#2478431, @aeubanks wrote:

In D94019#2478423, @dblaikie wrote:

In D94019#2478378, @aeubanks wrote:

Oh sorry, yeah this isn't NFC.

But I still don't think this needs a new test. We're going from a custom Clang implementation of adding passes to something more generic that's also more tested within LLVM.
So IMO we just need an end to end test, which we already have in unique-internal-linkage-names.cpp. The details are tested on the LLVM side.

Then could the end to end test be tightened up to demonstrate how this patch/the change in pass order has produced new/different/desired behavior?

It could be, but again I don't see the point. Rather than thinking about testing within individual changes, I think thinking about the overall state of testing regarding UniqueInternalLinkageNames after this change is better.
The only Clang test we need is to make sure PTO.UniqueLinkageNames is properly flipped, since that is the only thing Clang does regarding UniqueInternalLinkageNames. We have that Clang test in unique-internal-linkage-names.cpp (we could have a test that checks the PTO if it were serialized as you've said, but unique-internal-linkage-names.cpp is fine). It would fail if we didn't do PTO.UniqueLinkageNames = CodeGenOpts.UniqueInternalLinkageNames;.

Or if the functionality were implemented as it were before this patch - by explicitly adding the pass to the MPM in Clang, rather than by using UniqueInternalLinkageNames - to demonstrate & validate that this change had the desired effect, there should be a test case.

Yeah, if this code had been written with the PTO flag from the start, I probably wouldn't've considered/suggested/required this test to be written - but given the code was written that way, and is now being changed, I think that change should be tested.

Testing exactly what PTO.UniqueLinkageNames does is better done in LLVM. And that has already been done.

General detailed testing of the functionality should be done in LLVM, I agree - but testing that this Clang change does what's intended should be done in Clang where the change is made. If MPM and other flags were serialized with IR, we would test it that way (if we had a perfect serialization boundary - we probably would've previously been testing the pass sequence via some serialized form of the pass pipeline, perhaps, then changed to testing a serialized form of the PTO values - in lieu of that, we can either test the pass pipeline dump or choose as robust an end-to-end IR example as possible that is resilient to as many other changes to the LLVM implementation as possible (as, I assume, the existing end-to-end test is) while demonstrating the purpose/value of this change)

clang/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
   PTO.LoopInterleaving = CodeGenOpts.UnrollLoops;
   PTO.LoopVectorization = CodeGenOpts.VectorizeLoop;
   PTO.SLPVectorization = CodeGenOpts.VectorizeSLP;
   PTO.MergeFunctions = CodeGenOpts.MergeFunctions;
   // Only enable CGProfilePass when using integrated assembler, since
   // non-integrated assemblers don't recognize .cgprofile section.
   PTO.CallGraphProfile = !CodeGenOpts.DisableIntegratedAS;
   PTO.Coroutines = LangOpts.Coroutines;
+  PTO.UniqueLinkageNames = CodeGenOpts.UniqueInternalLinkageNames;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI(CodeGenOpts.DebugPassManager);
   SI.registerCallbacks(PIC);
   PassBuilder PB(CodeGenOpts.DebugPassManager, TM.get(), PTO, PGOOpt, &PIC);
 
   // Attempt to load pass plugins and register their callbacks with PB.
   for (auto &PluginFN : CodeGenOpts.PassPlugins) {
if (!CodeGenOpts.DisableLLVMPasses) {
     } else if (IsThinLTO) {
       MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
     } else if (IsLTO) {
       MPM = PB.buildLTOPreLinkDefaultPipeline(Level);
     } else {
       MPM = PB.buildPerModuleDefaultPipeline(Level);
     }
 
-    // Add UniqueInternalLinkageNames Pass which renames internal linkage
-    // symbols with unique names.
-    if (CodeGenOpts.UniqueInternalLinkageNames)
-      MPM.addPass(UniqueInternalLinkageNamesPass());
-
     if (!CodeGenOpts.MemoryProfileOutput.empty()) {
       MPM.addPass(createModuleToFunctionPassAdaptor(MemProfilerPass()));
       MPM.addPass(ModuleMemProfilerPass());
     }
   }
 
   // FIXME: We still use the legacy pass manager to do code generation. We
   // create that pass manager here and use it as needed below.