[PATCH] D82673: [ModuloSchedule] Make PeelingModuloScheduleExpander inheritable.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D82673#2126023 <https://reviews.llvm.org/D82673#2126023>, @hgreving wrote:

> In D82673#2125936 <https://reviews.llvm.org/D82673#2125936>, @dblaikie wrote:
>
> > Adding an extension point with no testing inside LLVM seems a bit problematic - is there no existing target that could use this? Or a reasonable scale of unit test that could exercise this extension point without a full-blown target?
> >
> > (also this produced a warning about the fact that this type doesn't have a virtual dtor - was fixed by @jfb in c7586444ca787c3845ac4ad0bd603709f2abbb0f <https://reviews.llvm.org/rGc7586444ca787c3845ac4ad0bd603709f2abbb0f> )
>
>
> The change is basically NFC. It was made in order to allow us using the upstream pass by inheriting from it.


Why does "expand" need to be virtual to inherit from it? The current caller in-tree doesn't call this function in a setting that would be applicable to polymorphism, for instance - it creates a concrete PeelingModuloScheduleExpander instance and calls "expand" directly on that.

>   I'm not sure what test you would write, are you thinking about a test that tests protected vs. private?

Nah - I was assuming the in-tree use was structured in a way that, while there might be only one implementation now, there might be some separation between construction and usage - such that maybe that code (that had usage separate from construction) could be unit tested with, say, a stub derived class to show the overriding behavior being exercised. But since usage is not separate from construction that doesn't seem to be possible/reasonable.

If out-of-tree uses would look much like the in-tree use, it doesn't seem to me like virtuality is required - the derived class would define its own expand function (& it could derive privately from the base class to hide the original implementation) and users would call that one.
If out-of-tree code wants to use dynamic polymorphism here, it seems like it could do so by introducing a small class deriving privately and exposing a virtual function.

But without any use in-tree that looks at least /roughly/ like the sort of thing that would benefit from dynamic polymorphism - I think that's best left out of tree.

>   Hexagon is the only upstream target upstream using the modulo scheduler. Full disclosure, we're a bit short staffed and are not in a position writing Hexagon tests. We _would_ like to use the upstream pass and when the time comes, add our improvements to LLVM upstream. Making the peeling modulo scheduler inheritable is a reasonable interface change, because different subtargets have different peeling requirements while sharing common semantics. 
> 
> The change simply allowed for the entire interface to be inheritable. Reality is, if the change is unwanted, we would end up downstreaming the pass entirely and developement on the upstream pass would stop - for now. If you're ok with the change, I think it's beneficial to everybody at this point. LMK, thanks for considering

Who else has a vested interest in having this in-tree? Perhaps they could be CC'd in this review to discuss this.

& would a small virtual shim out-of-tree allow you to work downstream while still having this pass in-use upstream?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82673