[llvm-commits] [PATCH][FastMath, InstCombine] Fadd/Fsub optimizations

[Duncan Sands]

Hi Shuxin,

On 12/12/12 18:53, Shuxin Yang wrote:
> "XYX say that patch is complicate, then the patch has to be complicated".
> I have hard time in understanding this logic. Please give some evidence!

you seem to have a problem with criticism.  You went ballistic when I fixed
a problem in reassociate differently to your proposed fix.  Your reaction to
Chandler's review of one of your recent patches was dismissive.  Your reaction
to Eli saying that your patch is complex and too hard to understand was also
to dismiss his remarks as unfounded, rather than to ponder on them and see if
maybe there is indeed some way to do it better.  Code review is essential to
producing quality code, but it requires that you listen to what others are
saying.  We are trying to help you to improve your patches, but you react more
like we were trying to hinder you rather than help you.  You don't have to
agree with everything we say, but you also shouldn't dismiss all suggestions
out of hand.

Ciao, Duncan.

PS: As I clearly said in my previous email, I didn't read your patch.  I was
commenting on your attitude not your code.

>
> I believe my change is simple enough.
> It has only 800 lines of code for addition optimization. If were not for
> floating point,
> the complexity can be cut by half.
>
> The diff has about 800 lines, the the code has some 730 lines. Among the them,
> 150 lines for Coefficient. Another 150 lines of code are for FAddend. These two
> classes are simplified version of the std Complex class in C++ Newbie book.
>
> For the rest code, at lest 1/5 are comments, space etc. Please tell me why it
> complex, and where it can
> be simplified.
>
>
> On 12/12/12 8:34 AM, Duncan Sands wrote:
>> Hi Shuxin,
>>
>> >> My high-level concern with this patch is basically that this is a lot
>>>> of code, the algorithm is complex, and the implementation is hard to
>>>> reason about; I'm trying to find places where can simplify if
>>>> possible.  And I'm immediately skeptical of any performance claim
>>>> without numbers.
>>> Some 700 lines of code is nothing.
>>> Among them 650 lines are about utility, the core has about 50 lines.
>>
>> I didn't read your patch but I have complete confidence in Eli's judgement, and
>> I think you should take whatever he says very seriously too.  If he says that
>> your patch seems too complex please consider the possibility that he is right,
>> and work hard to simplify, refactorize and clarify the code.
>>
>> Ciao, Duncan.
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>