[llvm-dev] RFC: CfgTraits/CfgInterface design discussion

Sat Oct 24 15:13:21 PDT 2020

Responding to another comment:

>>! In D83088#2351225, @mehdi_amini wrote:
>>>! In D83088#2350287, @nhaehnle wrote:
>> Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have a general rule that patches can be reverted if they're obviously broken (e.g. build bot problems) or clearly violate some other standard. This is a good rule, but it doesn't apply here. If you think it does, please state your case in the email thread that I've started on llvm-dev for this very purpose. Just one thing:
>
> I strongly disagree: the bar for post-review commit is not lower than pre-commit review.
>
> Again: please revert when someone has a concern, including a *design* concern with you patch.

What the developer policy _actually_ says (notwithstanding your
underhanded attempt to rush a change in
https://reviews.llvm.org/D89995) is that: "if substantial problems are
identified, it is expected that the patch is reverted and fixed
offline". This is also how things tend to play out in practice:
problems are fixed in-tree, without a revert, where possible --
because that's how decent human beings operate. So I'd appreciate it
if you stopped playing policy games and started to discuss the case on
its actual merits.

Has a substantial problem truly been identified?

It seems to me that what is really there in David's comments is a
mixture of two things. There are detail questions that are best
addressed in-tree (I discuss this some more below). And there is the
question of using dynamic polymorphism to write algorithms that are
generic over the type of IR. David is clearly at least uneasy about
this, and it is really only this question that would merit a revert.
So let's focus on that.

I think there are two options:

Maybe David wants to forbid this use of dynamic polymorphism outright.
I think this is unacceptable, so in that case, it seems we have no
choice but to start a formal process for resolving contentious
decisions. If that is really the path we need to go down, then sure,
that constitutes a substantial problem and I will accept it if people
still want me to revert the change while that process is ongoing. (I
would still ask people to _please_ be good citizens and allow us to
make upstream progress in AMDGPU as we usually do while this is
happening -- I explain my reasoning a bit more below -- but I'd accept
it based on the rules that are in effect today. Invoking the formal
process should give all participants the confidence that the question
doesn't just end up dropped on the floor, and that the in-tree status
of the code wouldn't implicitly favor either side of the discussion.)

Maybe David merely feels uncertain about whether dynamic polymorphism
is the best path here. In that case, I frankly don't see how reverting
helps the process in any way. It's fine for people to be uncertain
about something, but in that case we should err on the side of making
progress -- that's usually the best way to learn and get more
information!

(And of course maybe somebody else wants to weigh in on the technical
questions as well.)

>> P.S.: It's easy to miss on Phabricator, but there is already a long stack of patches which build on this
>
> (this is part of my previous point).

It cuts both ways, actually. When making a design like this one, there
are a number of reasons why you really should have a use of the design
early on as a good software developer. The two main ones:

1. Without attempting to use the design, you have no way of knowing
whether it's any good -- having the use case is pretty much a
prerequisite for iterating on the design.
2. Without having a use case, reviewers will likely just tell you to
come back when you have one, because of 1.
3. We have _real_ problems that we're solving and need to make
progress on. I can't just sit on my hands waiting for some discussion
that people don't pay attention to, and having the dynamic genericity
greatly simplified making this progress so far. So going ahead with
work based on this patch was the right decision. (FWIW, I consider the
ability to use dynamic polymorphism here to be such a great benefit
that doing this will have been a net positive _even if_ I'm forced to
remove it eventually, because turning things into templates after the
fact isn't actually too difficult; it would just hurt
maintainability.)

For the LLVM community, the fact that this code exists already today
is a benefit. Having all of the code in-tree (both this particular
change and its uses) would allow us to iterate on the interfaces while
being able to see how real users of them are affected, as opposed to
mere toy examples. Of course, this is subject to those further changes
being accepted to the tree, and not all of them are at that level of
quality yet, but many of them are.

I understand that some people may be scared that once the code is in
there, it'll be in there forever. But this should only be a concern
for those who object to dynamic polymorphism on principle. As I wrote
above, turning things into templates after the fact is _much_ easier
than going in the other direction or writing the code as templates
from scratch, and I have to make progress on this work anyway. What
this means is that _if_ the LLVM community really were to decide that
this use of dynamic polymorphism is unacceptable (why?!?), I'm already
implicitly signed up to doing this templatification work. And that's
okay -- cost of business of working open-source upstream -- but what's
not okay is if LLVM makes upstream development systematically hard.

Consider:

1. We usually develop AMDGPU upstream as much as possible -- most of
our LLVM developers actually do work directly on upstream and not on a
vendor tree -- and we want to continue doing so.
2. A large amount of other work in AMDGPU implicitly or explicitly
depends on it. Most importantly, GlobalISel needs a divergence
analysis on MachineIR. This means that other developers, who usually
work upstream, depend on this work being available. If you insist on
reverting the change, you may be forcing us to build up new internal
processes for doing less work upstream. I would _really_ like us to
avoid that.

So I would ask you to allow us to make this progress in-tree with the
understanding that the changes that would be required to excise the
dynamic polymorphism again are fairly limited and I'm signed up to
doing them if needed. There really is a question here of how friendly
to upstream development you want LLVM to be, and the timescales that
are involved need to be taken into consideration.

Bottom line: please, let's just figure out where we stand on the
question of dynamic polymorphism for algorithms that are generic over
different types of IR, and then see how we go from there.

Cheers,
Nicolai

--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.