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

Sat Oct 24 15:50:11 PDT 2020

On Sat, Oct 24, 2020 at 3:13 PM Nicolai Hähnle <nhaehnle at gmail.com> wrote:

> 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)

Nicolai,  I don't appreciate the way you are mischaracterizing this. There
is no rush for a topic that was discussed 4 years ago and that was about
codifying the *existing practices*. If there was any rush I would have
landed it immediately after getting approval from Renato and wouldn't have
brought it up to llvm-dev@ directly.

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,

 Sorry I have a hard time reading this: you quote a sentence saying that
the patch should be reverted and follow by saying that problems are fixed
in-tree. I see a contradiction here.

> without a revert, where possible --
> because that's how decent human beings operate.

Again, I don't appreciate your implied personal attacks here: do we have to
agree with you to be a decent human being? This is violent, please stop.

> 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?
>

Someone asked you to revert because of design concerns, and you are
refusing. At this point this is a policy problem, I don't want to engage in
your attempt to redirect what is an easy process towards the details of
whether David is right or wrong or what is the best implementation path for
this particular patch right now. From this point of view, everything below
is irrelevant to the revert request: you can bring this up with David on
LLVM-dev, discuss it, make it an RFC in the end, and in the ideal case (for
you) recommit the patch as is.
In the meantime: please revert.

Thanks,

-- 
Mehdi

>
> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201024/a15880a7/attachment.html>