[llvm-commits] [llvm] r168931 - in /llvm/trunk: include/llvm/Target/TargetTransformImpl.h include/llvm/TargetTransformInfo.h lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86ISelLowering.h lib/Target/X86/X86TargetMachine.h lib/Transforms/Scalar/LoopIdiomRecognize.cpp test/Transforms/LoopIdiom/popcnt.ll

[Shuxin Yang]

On 01/08/2013 10:36 PM, Chandler Carruth wrote:
> On Mon, Jan 7, 2013 at 10:56 AM, Shuxin Yang <shuxin.llvm at gmail.com 
> <mailto:shuxin.llvm at gmail.com>> wrote:
>
>     Hi, Michael:
>
>        Thanks!  I actually have that mail and I also replied 50% of
>     his mails, and I also fixed one typo.
>     I ignore the rest 50% of his mail because I don't think he
>     understand population-count.
>
>
> Shuxin, I know that you are new to LLVM project (and goodness knows 
> we're a strange crew), but ignoring code review because you don't 
> think the code reviewers understand your code isn't really a viable 
> strategy.
>
> First off, code reviews are critically important to the long-term 
> health of the project. As part of that, it is important to cultivate, 
> encourage, and help code reviewers who don't understand your code gain 
> an understanding and become more involved in the project. In fact, 
> that is part of being part of an open source project: helping others 
> become more active and involved. If someone doesn't understand the 
> code you're working on, you should explain it, or if you can re-direct 
> them to web pages, articles, and books on the subject so they can 
> learn and give increasingly relevant code review.
Chandler, I did not "ignore" code reviewers' comment. In the contrary,
I strictly follow (almost) every single comment reviewers' figured out.
However, I had hard time in following your comment, something like "what 
if the integer has 24 bit"?

>
> Secondly, I have some understanding population-count, so I don't think 
> my understanding is really the issue at hand.
>
> I haven't responded to your original email for two reasons:
> 1) You hadn't actually addressed or responded to the other half of my 
> comments. I had assumed you were working on the actual bugs, and 
> didn't want to divert the discussion until the bugs were fixed. 
> Perhaps I should have directly replied.
> 2) Many of your responses simply state that you disagree and are, 
> frankly, dismissive. Whether you agree or not, you cannot simply 
> dismiss code review comments. If you disagree strongly, explain why 
> and try to figure out a solution both you and your reviewer are happy 
> with. If you don't disagree strongly, consider just making the 
> change--it's a nice way to show you appreciate them taking the time to 
> review your code. At the end of the day, when two contributors 
> disagree strongly on an issue, we usually try to get a reasonable poll 
> of different contributors' opinions and if necessary weight them by 
> their contributions to the project.
>
> Nevertheless, I will respond to a couple of the actual questions you 
> raised since that seems to be what you are waiting for.
>
> A few more comments on the rest of your email:
>
>     I only focused on the correctness part.
>
>
> This simply isn't acceptable in the LLVM community. Code review, *all* 
> of it, is critical. Coding style is part of it, but so is design 
> review. I had comments and suggestions in both areas which have not 
> been addressed, and consequentially maintaining and extending this 
> code is harder for every other contributor.
By "focusing on the correctness part", I mean I ignore your comments 
reading the potential correctness problem.
As far as I can tell, they are absolute not a problem.
About 80% of comments are based on your misunderstanding of 
population-count and the code.

>
> If either style or design patterns don't make sense to you, I'm happy 
> to discuss it further, but I will need you to help me understand the 
> part that is confusing or unclear. I would also encourage you to look 
> at some of the other code within the LLVM codebase, some of the 
> commits of long-time contributors such as Chris, Dan, Duncan, or 
> others. These might help you understand some of design and style 
> principles of the project.
Basically, I'm open to any design pattern, as I said before in 
engineering world, there is no absolute true of false.
In your previous mail, you figure out some design issue, how come we 
need to a helper class?
How come the class need "Ncl_" (std for non-countable loop) prefix. I 
believe these issues are
really in a gray area.  Like the 2nd issue. If I don't use "Ncl_" 
prefix, how can I differentiate the
patterns in countable loop and in non-countable loop?  Ok, we can 
certainly replace the prefix
with other fancy names, will it make things tons better?

>     As far as I can tell from the assertion, I believe the
>     case I miscompiled is a real pop-count  -- it does count
>     population in the mean time
>     do something else. Unfortunately, Chanlder was reluctant to show
>     me the minimal testing case,
>     and only clam "it is not pop-count, but looks similar to it".  
>     Actually, my implement had
>     another problem he didn't figure out.
>
>
> I had very little time when debugging this. Sorry I didn't give you a 
> complete test case. However, I was able to point at the exact bug in 
> question as well as how to fix it, which should have helped you both 
> fix and test the fix. You also would likely have written more popcnt 
> test cases than I and might be able to do so more easily.
You don't have to debug.  You just need to report the problem with a 
testing cases.
Isn't it a standard practice?

Unfortunately, you didn't follow this practice, you believe you find all 
the problems. You didn't.
Since I didn't have testing case to verify, I miss the 2nd problem, and 
piss you off.
You promptly revert my changes...

Now that it was my problem, I have to accept it, and I don't want to 
retaliate either:-)

But,  I hope you will be nice to other folks. Has anybody reverted your 
change to SROA,
which has been buggy for a awful long time?  What's point to make the 
community so hostile?

>
>     Among all the stylistic issues, one issue I'm not quite sure. It
>     was about enum definition
>     I copy the style from somewhere else in LLVM, and I check the
>     coding style std, I don't find
>     any words about it.  If you find a canonical way to define enum,
>     please let me know.
>
>
> From the coding standards document[1]:
> "Unless the enumerators are defined in their own small namespace or 
> inside a class, enumerators should have a prefix corresponding to the 
> enum declaration name. For example, enum ValueKind { ... }; may 
> contain enumerators like VK_Argument, VK_BasicBlock, etc."
>
> [1]: 
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Ok, I will fix this style problem,
and I remember there are couple of space I need to remove.
I will fix these problem in the following few days, maybe tomorrow.



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130108/55d583b0/attachment.html>