[llvm-commits] [PATCH][Fastmath, Instcombine] Enhancement of Fdiv

[Shuxin Yang]

Hi, David:

    Thank you so much for your feedback. See the comment bellow.

On 1/11/13 2:47 PM, dag at cray.com wrote:
> Shuxin Yang <shuxin.llvm at gmail.com> writes:
>
>>      1) reciprocal.patch:
>>          implement reciprocal rule:
>>           1.1   X/C => X *(1/C) if 1/C is neither special value nor
>> denormal.
>>
>>         This rule could incur noticeable rounding errors (sometimes up
>> to 0.001% -- one testing case
>>        in pollybench suite see a diff between 1.003 and 1.004, and the
>> "huge" difference is caused
>>        by a single transformation X/1000 => X * 0.001).
> Is this rule controlled by strict fp or other options?  Users should be
> able to disable it for precision.
Sorry, I forget to mention, it is enabled only if the instruction is 
flagged with "allow reciprocal".
For all rest rules, they must be marked as "fast-math".

>
>>     2. fdiv.patch implement a slew of fdiv related rules:
>>
>>        2.1). (X/C1) / C2 => X * (1/(C2*C1))
> Why not do (X/C1) / C2 => X / (C1 * C2) (under switch control) and let
> 1.1 clean it up if allowed by the previously-explained user switch?
You are absolutely right!
Actually, I want to try all the other rules in the form of (....)/C 
before the reciprocal rule.
i.e the reciprocal rule is the last resort.  But, I have to separate the 
reciprocal rule and
rest rules to make reviewers happy -- I guess  reviewers might ask me to 
separate the change
since the reciprocal has negative impact to the precision.

>
>>        2.2). X*C1 / C2 => X * (C1/C2)
> This will need a strict or similar switch control for fp arithmetic.
>
>>        2.3). (X/Y)/Z = > X/(Y*Z)  (at least one of Y and Z is symbolic
>> value)
>>        2.4). Z/(X/Y) = > (Z*Y)/X
>>        2.5). C1/(X*C2) => (C1/C2) / X
>>        2.6). C1/(X/C2) => (C1*C2) / X
>>        2.7) C1/(C2/X) => (C1/C2) * X
> And all of these too.
>
> Every single one of these transformations can change fp arithmetic
> answers and thus we must have a way to disable them on the command line.
>
>                                 -David