hoy added a comment. 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. 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
