[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#2478308 <https://reviews.llvm.org/D94019#2478308>, @hoy wrote:

> In D94019#2478284 <https://reviews.llvm.org/D94019#2478284>, @dblaikie wrote:
>
>> In D94019#2478277 <https://reviews.llvm.org/D94019#2478277>, @hoy wrote:
>>
>>> In D94019#2478253 <https://reviews.llvm.org/D94019#2478253>, @dblaikie wrote:
>>>
>>>> In D94019#2478206 <https://reviews.llvm.org/D94019#2478206>, @hoy wrote:
>>>>
>>>>> In D94019#2478048 <https://reviews.llvm.org/D94019#2478048>, @dblaikie wrote:
>>>>>
>>>>>> In D94019#2478047 <https://reviews.llvm.org/D94019#2478047>, @hoy wrote:
>>>>>>
>>>>>>> In D94019#2478045 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94019