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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to