[PATCH] D94019: Switching Clang UniqueInternalLinkageNamesPass scheduling to using the LLVM one with newpm.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D94019#2478431 <https://reviews.llvm.org/D94019#2478431>, @aeubanks wrote:

> In D94019#2478423 <https://reviews.llvm.org/D94019#2478423>, @dblaikie wrote:
>
>> In D94019#2478378 <https://reviews.llvm.org/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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94019