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

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.
I only focused on the correctness part.  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.

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. Thanks in advance!

Shuxin


On 1/7/13 10:30 AM, Michael Ilseman wrote:
>
> On Jan 7, 2013, at 10:08 AM, Shuxin Yang <shuxin.llvm at gmail.com 
> <mailto:shuxin.llvm at gmail.com>> wrote:
>
>> Please be specific! What are the problematic styles?
>
> Chandler's first email in this thread (the really long one!) had a lot 
> of good stylistic suggestions. 
> http://llvm.org/docs/CodingStandards.html has the coding standards 
> guide, and many of the things he pointed out are from it. If you lost 
> that email, I can resend it to you.
>
>> Your previous mail about this issue full of technical mistake about 
>> population count.
>> I have to ignore it.
>>
>> On 1/7/13 12:42 AM, Chandler Carruth wrote:
>>> On Fri, Dec 7, 2012 at 6:35 PM, Chandler Carruth 
>>> <chandlerc at google.com <mailto:chandlerc at google.com>> wrote:
>>>
>>>          Some "style" is the combination of different reviewer's
>>>         tastes.  Some reviewers' taste are just opposite to you
>>>         personal opinions.
>>>
>>>
>>>     Within the LLVM project, style is part of code review. My
>>>     comments below about the style issues with your code are code
>>>     review that needs to be addressed, even if they don't seem like
>>>     important style issues to you.
>>>
>>>
>>> Shuxin, I hate to dig this back up, but the style of this code 
>>> really does need to be addressed. I'm having to make changes to this 
>>> code as I refactor other parts of LLVM, and it is substantially 
>>> harder to read, modify, and generally maintain. That is why we have 
>>> code review even for style and design, and why it is so important 
>>> that these review comments are addressed.
>>>
>>> Please prioritize resolving these issues with this code. It has been 
>>> in the tree in a form that is well below the expected code quality 
>>> of LLVM for a month now. I think that is too long.
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

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