[clang] 24ab9b5 - Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via cfe-commits]

Am Mi., 8. Jan. 2020 um 17:20 Uhr schrieb Eric Christopher <echristo at gmail.com>:
>
>
>
> On Wed, Jan 8, 2020 at 3:07 PM Michael Kruse <llvm-commits at meinersbur.de> wrote:
>>
>> Am Di., 7. Jan. 2020 um 18:57 Uhr schrieb Eric Christopher via
>> cfe-commits <cfe-commits at lists.llvm.org>:
>> >> Is there anything I should have done? Probably reaching llvm-dev before
>> >> commiting. Reaching the right reviewers has always been a challenge to me, I had
>> >> hoped that the mail to llvm-dev would trigger some subscription :-)
>> >
>> >
>> > Hrm. Probably finding some different reviewers, but I can't fault your attempts here. Usually you can look at the last few people to make substantial work in an area and loop them in via git log. :)
>>
>> In my experience adding more reviewers usually rarely to more people
>> looking into the patch. Especially the people that make substantial
>> work in an area and/or are code owners are added as reviewers to very
>> many commits and cannot look into all of them.
>
>
> As one of those added to a lot of patches I have a lot of sympathy here. The answer isn't to avoid adding people with expertise in an area and no one on the approval list had expertise or had recently hacked on the code.

For some parts you won't find people that recently worked on the code
other than refactoring, such as the legacy pass manager. Together with
Tobias Grosser, I worked out the mechanism that statically linked
Polly into tools which was generalized in this patch. I am not sure
what else would be required to approve this patch.


>
>>
>> Note that in the patch in question, there are comments from people not
>> in the reviewer list. For example, Mehdi with whom I also talked about
>> this patch at the last LLVM DevMtg.
>>
>
> Yes, but none of them ack'd it.

Mehdi did in the comments (and personally at the DevMtg). He just did
not approve the patch because I still had concerns. By coding policy,
only one approval is required.



>> >> >  ; CHECK-EP-VECTORIZER-START-NEXT: Running pass: NoOpFunctionPass
>> >> > +; CHECK-EXT: Running pass: {{.*}}::Bye on foo
>> >> >
>> >> > Why is this running on every test of the pass manager? It should be an example
>> >> > run in the examples directory and not on by default? Same for every other PM
>> >> > test. This seems like a bug?
>> >>
>> >> It's not. When the examples are active and if the appropriate cmake flag is set
>> >> (which is not the case by default), the pass is linked in statically, and is run
>> >> in the default pipeline. The CHECK-EXT prefix is disabled otherwise. That's one
>> >> of the configuration I did test :-)
>> >>
>> >
>> > I really don't think this is ideal. The examples directory shouldn't affect tests being run or not or in what way. Can we back this part out and talk about it a bit more? I don't think we should need to do this to test the functionality.
>>
>> In a previous discussion [1] we discussed adding tutorial code to
>> repository which naturally should also be tested to avoid bit-rot. How
>> do you suggest to testing of examples should work? Note that LLVMHello
>> is also tested, but not in the example directory.
>
>
> llvm/tests/examples?

The point is, we don't have an established pattern of how to test
examples yet, and I would also like that we develop one. I also think
this was not in the scope of this patch.


Michael