llvm dev - Oct 2020 - RFC: CfgTraits/CfgInterface design discussion

Also, if this hasn't happened already, I would recommend some 1-on-1 (at
least over chat, or if possible video call) between Nicolai and Dave. In
the past, I have found this to fairly quickly come to consensus about
design direction (though of course please update any relevant threads with
the takeaways of such private discussions!).

-- Sean Silva

On Mon, Oct 26, 2020 at 1:05 PM Eric Christopher via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hi Nicolai,
>
> I've been watching this and the associated review that +Mehdi AMINI
> <joker.eph at gmail.com> brought up as far as the process.
>
> I think you should revert this patch and anything dependent upon it until
> the review is complete. Dave has many good points in his review and while
> you pinged there needs to be resolution before applying. In particular,
> he's probably the most active reviewer in exactly this space right now
and
> is obviously also the right reviewer for this patch.
>
> Please revert immediately and thanks. I'm sorry the patch has gotten
> contentious, but this is a fairly major overhaul and it does happen
> sometimes.
>
> Thanks.
>
> -eric
>
>
>
> On Mon, Oct 26, 2020 at 2:46 PM Renato Golin via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> On Sat, 24 Oct 2020 at 23:13, Nicolai Hähnle via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> 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 this ends up being the case, then you *must* revert the patch until
>> the matter can be solved.
>>
>> But as I said before, I would have reverted it a long time ago.
>>
>> (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.)
>>>
>>
>> The AMD backend doesn't trump a high-level CFG design decision that
>> affects *all* back-ends, front-ends and middle-ends.
>>
>> If you progress your AMD work on your current assumption and it turns
out
>> people decide against it you will have to revert *all* of it, which is
a
>> lot more substantial (and a potentially dangerous merge) than just this
CFG
>> change.
>>
>> I'd also strongly advise people reviewing the remaining work from
>> approving the patches until this matter can be resolved. This is not a
>> trivial issue and can have further consequences than just a simple
revert.
>>
>> And please, do not assume what being a good citizens is. We can all be
>> good citizens and still overwhelmingly disagree with each other, as
long as
>> we keep it civil.
>>
>> I see no evidence of lack of civility from either David or Mehdi. On
the
>> contrary, they're being extremely kind and patient.
>>
>> cheers,
>> --renato
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201026/3fe5298f/attachment.html>

I agree with Eric here.  There is clearly controversy w.r.t. this patch and
regardless of what the formally written policy is, it seems best to revert this
and resolve the outstanding issues.  Keeping it in tree will cause more things
to be built on top of it, and over constrains the discussion about getting to
the best possible design.

Sean’s point is also excellent: often it is best to pop out of email and into a
higher bandwidth communication form (voice, video chat, etc) when hitting
controversy like this.

-Chris

> On Oct 26, 2020, at 4:26 PM, Sean Silva via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> Also, if this hasn't happened already, I would recommend some 1-on-1
(at least over chat, or if possible video call) between Nicolai and Dave. In the
past, I have found this to fairly quickly come to consensus about design
direction (though of course please update any relevant threads with the
takeaways of such private discussions!).
> 
> -- Sean Silva
> 
> On Mon, Oct 26, 2020 at 1:05 PM Eric Christopher via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> Hi Nicolai,
> 
> I've been watching this and the associated review that +Mehdi AMINI
<mailto:joker.eph at gmail.com> brought up as far as the process.
> 
> I think you should revert this patch and anything dependent upon it until
the review is complete. Dave has many good points in his review and while you
pinged there needs to be resolution before applying. In particular, he's
probably the most active reviewer in exactly this space right now and is
obviously also the right reviewer for this patch.
> 
> Please revert immediately and thanks. I'm sorry the patch has gotten
contentious, but this is a fairly major overhaul and it does happen sometimes.
> 
> Thanks.
> 
> -eric
> 
> 
> 
> On Mon, Oct 26, 2020 at 2:46 PM Renato Golin via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> On Sat, 24 Oct 2020 at 23:13, Nicolai Hähnle via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> 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 this ends up being the case, then you *must* revert the patch until the
matter can be solved.
> 
> But as I said before, I would have reverted it a long time ago.
> 
> (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.)
> 
> The AMD backend doesn't trump a high-level CFG design decision that
affects *all* back-ends, front-ends and middle-ends.
> 
> If you progress your AMD work on your current assumption and it turns out
people decide against it you will have to revert *all* of it, which is a lot
more substantial (and a potentially dangerous merge) than just this CFG change.
> 
> I'd also strongly advise people reviewing the remaining work from
approving the patches until this matter can be resolved. This is not a trivial
issue and can have further consequences than just a simple revert.
> 
> And please, do not assume what being a good citizens is. We can all be good
citizens and still overwhelmingly disagree with each other, as long as we keep
it civil.
> 
> I see no evidence of lack of civility from either David or Mehdi. On the
contrary, they're being extremely kind and patient.
> 
> cheers,
> --renato
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201031/eee13ed7/attachment.html>

On Sat, Oct 31, 2020 at 12:32 PM Chris Lattner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> I agree with Eric here.  There is clearly controversy w.r.t. this patch
> and regardless of what the formally written policy is, it seems best to
> revert this and resolve the outstanding issues.  Keeping it in tree will
> cause more things to be built on top of it, and over constrains the
> discussion about getting to the best possible design.
>
> Sean’s point is also excellent: often it is best to pop out of email and
> into a higher bandwidth communication form (voice, video chat, etc) when
> hitting controversy like this.
>
Yep, Nicolai's reverted the patch (see:
http://lists.llvm.org/pipermail/llvm-dev/2020-October/146107.html ) - the
conversation's got a bit fragmented between the review thread, new RFC
thread, and rename of that thread) and Alina, Nicolai, and myself chatted
offline on Thursday - think we came to some pieces of common understanding.
Nicolai's going to work up a new RFC (
https://reviews.llvm.org/D83088#2365320 ,
http://lists.llvm.org/pipermail/llvm-dev/2020-October/146258.html )  we can
hopefully get a few eyes on/clarify some things to make progress.

- Dave

>
> -Chris
>
>
> On Oct 26, 2020, at 4:26 PM, Sean Silva via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Also, if this hasn't happened already, I would recommend some 1-on-1
(at
> least over chat, or if possible video call) between Nicolai and Dave. In
> the past, I have found this to fairly quickly come to consensus about
> design direction (though of course please update any relevant threads with
> the takeaways of such private discussions!).
>
> -- Sean Silva
>
> On Mon, Oct 26, 2020 at 1:05 PM Eric Christopher via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> Hi Nicolai,
>>
>> I've been watching this and the associated review that +Mehdi AMINI
>> <joker.eph at gmail.com> brought up as far as the process.
>>
>> I think you should revert this patch and anything dependent upon it
until
>> the review is complete. Dave has many good points in his review and
while
>> you pinged there needs to be resolution before applying. In particular,
>> he's probably the most active reviewer in exactly this space right
now and
>> is obviously also the right reviewer for this patch.
>>
>> Please revert immediately and thanks. I'm sorry the patch has
gotten
>> contentious, but this is a fairly major overhaul and it does happen
>> sometimes.
>>
>> Thanks.
>>
>> -eric
>>
>>
>>
>> On Mon, Oct 26, 2020 at 2:46 PM Renato Golin via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> On Sat, 24 Oct 2020 at 23:13, Nicolai Hähnle via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> 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 this ends up being the case, then you *must* revert the patch
until
>>> the matter can be solved.
>>>
>>> But as I said before, I would have reverted it a long time ago.
>>>
>>> (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.)
>>>>
>>>
>>> The AMD backend doesn't trump a high-level CFG design decision
that
>>> affects *all* back-ends, front-ends and middle-ends.
>>>
>>> If you progress your AMD work on your current assumption and it
turns
>>> out people decide against it you will have to revert *all* of it,
which is
>>> a lot more substantial (and a potentially dangerous merge) than
just this
>>> CFG change.
>>>
>>> I'd also strongly advise people reviewing the remaining work
from
>>> approving the patches until this matter can be resolved. This is
not a
>>> trivial issue and can have further consequences than just a simple
revert.
>>>
>>> And please, do not assume what being a good citizens is. We can all
be
>>> good citizens and still overwhelmingly disagree with each other, as
long as
>>> we keep it civil.
>>>
>>> I see no evidence of lack of civility from either David or Mehdi.
On the
>>> contrary, they're being extremely kind and patient.
>>>
>>> cheers,
>>> --renato
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201031/6b63dc76/attachment-0001.html>