[Openmp-commits] [PATCH] D62393: [OPENMP][NVPTX]Mark parallel level counter as volatile.

Thu Jun 13 18:23:36 PDT 2019

ABataev added a comment.

In D62393#1542884 <https://reviews.llvm.org/D62393#1542884>, @hfinkel wrote:

> In D62393#1542863 <https://reviews.llvm.org/D62393#1542863>, @ABataev wrote:
>
> > In D62393#1542599 <https://reviews.llvm.org/D62393#1542599>, @hfinkel wrote:
> >
> > > In D62393#1542505 <https://reviews.llvm.org/D62393#1542505>, @ABataev wrote:
> > >
> > > > In D62393#1542465 <https://reviews.llvm.org/D62393#1542465>, @hfinkel wrote:
> > > >
> > > > > In D62393#1542213 <https://reviews.llvm.org/D62393#1542213>, @ABataev wrote:
> > > > >
> > > > > >
> > > > >
> > > > >
> > > > > ...
> > > > >
> > > > > >> Again, I see no reason to believe that everyone here isn't acting in good faith and working to create the software of the highest possible quality. Thanks!
> > > > > > 
> > > > > > Sorry, but Johannes did this. "The more patches go through with this kind of "review" the more "suspicious" this looks to me." It is his words. seems to me, he does not think that me or my colleagues are acting in good faith.
> > > > >
> > > > > Okay, I understand why you say this, but no. Frankly, Johannes was implying that the reviews were too lax (e.g., allowing combined patches with insufficient tests/documentation/etc.).
> > > >
> > > >
> > > > Only one patch was combined.
> > >
> > >
> > > I'm not sure that this is true. D55379 <https://reviews.llvm.org/D55379> was also. There might be others. I don't think that it's useful to conduct an audit in this regard. The important point is that there were a number of issues over time, some have been addressed, and others we're addressing right now.
> >
> >
> > Ok, you found another one committed half year ago. Do really think you can call it systematic and suspicious?
>
>
> If there is a systematic problem, then this would be one part among several. Regardless, we both agree that this is something to be avoided, so as far as I'm concerned, this problem has been addressed.
>
> > 
> > 
> >> 
> >> 
> >>> Tests are added to all the functional patches as soon as I discovered the testing infrastructure for libomptarget is configured already. And I myself insist on adding test cases for the functional patches, you can ask Doru about this.
> >> 
> >> I'm aware of a lot of the the history (and I recall being involved in the discussion in D60578 <https://reviews.llvm.org/D60578> regarding the tests). Nevertheless, we've somehow ended up with a library without sufficient documentation, and robust code reviews should have prevented that. It's not useful to assign blame, but it is something of which we should all be aware so that we can actively improve the situation doing forward.
> > 
> > I said already that we need a documentation. But I can't do everything myself. Plus, I'm not the original developer of the library, I'm just doing my best trying to improve it. Maybe you or Johaness can try to help with this and prepare a documentation? That would be a great help!
>
> Johannes and I can help with the documentation.

Good!

> 
> 
>> 
>> 
>>> 
>>> 
>>>> As for the documentation, yes, we lack of the documentation for libomptarget. This is a long story and it requires special work. I'm also not very happy with the current situation. But the documentation is a separate work. It requires special set of patches with the design decisions etc. Most of it follows the design of the original libomp, bit has some NVPTX specific parts.
>>> 
>>> Yes, we should have documentation covering the design decisions and other details, but I don't believe that treating this as a special task is productive. When a subsystem lacks good documentation, then we should improve that as we make changes to the code - just because the documentation is poor is not a good reason for not adding comments in the code now. It is, moreover, an even stronger argument for adding comments in the code now. Please do add whatever documentation is possible and associated with this change now.
>> 
>> We need the base where we could start. It is a separate work to prepare a base document which could be modified/upgraded with the next patches.
> 
> The lack of existing overall documentation does not prevent adding a comment explaining this particular variable, what it stores and why it is marked as volatile, now.

Sure, I'll add a comment for a variable describing its purpose and why it must be marked volatile.

> 
> 
>> 
>> 
>>> 
>>> 
>>>> 
>>>> 
>>>>> I think that all of these issues have now been covered here (and on D62199 <https://reviews.llvm.org/D62199>, etc.), and so I won't repeat them again. The reviewers (e.g., @grokos for the case of D62199 <https://reviews.llvm.org/D62199>) almost certainly believed that they were being helpful and moving the development process forward.
>>>> 
>>>> 
>>>> 
>>>>> The fact that the patches did not all follow our best practices, etc. makes Johannes "suspicious" that the patches could have reasonably been improved before being committed.
>>>> 
>>>> Which one do not follow "best practices"? The combined one? This is only one patch. NFC patches? They are refactoring patches and we already have the tests for them.
>>> 
>>> Again, it is not just one patch, and there have been a variety of issues. Some have already been addressed and others we're addressing here. The important point is to make sure that things improve over time, and that's exactly what we can do now.
>>> 
>>>> He did not try to understand the idea of the patches, instead he decided to treat them "suspicious" without any comments.
>>> 
>>> This statement is unproductive, and likely false. Johannes has spent a lot of time looking at the past patches and the code. If he sees a systematic problem, then there probably is one.
>> 
>> Let me disagree with you.
>> 
>>> 
>>> 
>>>> 
>>>> 
>>>>> It makes me suspicious of that too. But no one here believes that anyone was trying to subvert the system and produce an inferior result - it is very likely that everyone had, and continues to have, the best of intentions.
>>>> 
>>>> Maybe, just maybe, before starting treat someone's activity "suspicious" better to start to try to understand something? To read something, to ask the questions in the proper manner, etc.?
>>> 
>>> Again, this is unproductive, and false. I agree with Johannes could have been more polite at times - frustration can be stated directly, and that's almost always more productive - but let's be fair: there's a significant corpus of text where you're rude, dismissive, abrasive, and so on. I can provide links if you'd like, but I think you very well know that it's true. That should stop. Nevertheless, you expect everyone else to assume that you're working in good faith, have fairly considered and addressed their feedback, and so on. I suggest that you provide Johannes in this case this came courtesy you expect others to provide to you.
>> 
>> Sure, I was rude. When somebody blames me without any proves, I can be rude. I'm sorry for this! But it was caused by the unconstructive initial comment.
> 
> At this point, I feel as though everyone here as voiced their concerns about past practices and behavior. We understand how to address each others concerns going forward and maintain professional discourse. We should do so. I highly recommend that, at this point, we avoid future comments that a reader is likely to view as rude, condescending, dismissive, or confrontational. Any suboptimal code, documentation, etc. in previous patches can be improved by future patches, and that should be our focus going forward.
> 
>> 
>> 
>>> 
>>> 
>>>> 
>>>> 
>>>>> And, I'll point out, that we have indeed accomplished a lot in terms of OpenMP feature enablement, etc. At this point, we have more eyes on the process and we'll should all work together to produce an even-better result. The glass, in this case, really is half full.

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D62393