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

[Nicolai Hähnle via llvm-dev]

On Sun, Oct 25, 2020 at 12:50 AM Mehdi AMINI <joker.eph at gmail.com> wrote:
[snip]
>> 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.

That's not what the sentence is saying *unless* you just flat-out
assume that a substantial problem has been identified. That isn't
clear to me at all. Admittedly the adjective "substantial" is
subjective, which is why I was trying to get clarification here. I
don't know why you're trying to derail that.

And I'm not just being facetious or purposefully obnoxious. Even after
re-reading the relevant comments, it honestly isn't clear to me which
parts (if any) anybody considers substantial problems and which parts
are musings on potential alternatives or what.


[snip]
>> 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.

Let's disentangle those two things, please. The fact that I'm refusing
to revert (for now, pending further discussion and clarification!) is
not a substantial problem with the code change. So please stop
yourself for a moment, take a step back, and don't try to turn this
into some sort of Kafkaesque scenario along the lines of "well, now
that you refuse to revert, I'm going to use *that* as the argument for
why you must revert!" The LLVM community should be better than that.

Design concerns can be any number of things. They can be as simple as
"I prefer a different name for this class/method/variable" (just
rename it in-tree...) or as significant as "the entire approach is
unacceptable to me" (revert and back to the drawing board).


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

I have brought it up on llvm-dev, that's what this thread was
originally trying to do :)

Look, let's be clear on one thing here. The problem is that this is
happening after David was away without any sign of life for two
months. I appreciate that he apologized for that and I wouldn't blame
him in any case, I have problems with keeping up on reviews myself.
But it's a fact, and it makes what you're proposing here seem a bit
impractical. There seems to be a high chance that we are just going to
end up in the exact same place two months from now. What you claim to
be an easy process has already been proven to not be easy after all.

That's why I'm looking for reliable ways forward. Mind you, I still
accept the existing policies and would therefore revert if somebody
actually gave a clear reason. Yes, that would require some work of
distillation/synthesis being made by somebody who wants it to be
reverted. I find that a reasonable ask, because reverting would be
pointless anyway if the person asking for it is unwilling to engage in
review discussion.

In my last emails I have been trying to find ways of re-establishing
trust that progress will actually be made. For example, that's why I
was trying to get a feeling for whether people want a formal process
for contentious decisions, and if so, what decision we'd even be
talking about. Getting clarity on what the perceived problems with the
patch really are is an important part of that. It's counterproductive
of you to try to shut that down.

(FWIW, I haven't heard anything from David in a while so I'm also
trying to reach out in private, just in case that's preferred.)

Cheers,
Nicolai






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



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