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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
