[llvm] r371518 - [InstCombine] Use SimplifyFMulInst to simplify multiply in fma.

[Florian Hahn via llvm-commits]

> On 10 Sep 2019, at 21:41, Philip Reames <listmail at philipreames.com> wrote:
> 
> 
>> On 9/10/19 1:29 PM, Florian Hahn wrote:
>>> On Tue, Sep 10, 2019 at 8:39 PM Philip Reames <listmail at philipreames.com> wrote:
>>> Wait, is this correct?  I thought that FMA had different rounding
>>> behavior than fadd/fmul.  Is it legal to take the simplified mul and
>>> simple feed it as an operand to a fadd?
>>> 
>> Yes, I think if SimplifyFMulInst would return a new `simplified` fmul
>> instruction, than the transform as it is would introduce additional
>> rounding error. For example, a few lines above, we have the following
>> transform: fma fabs(x), fabs(x), z -> fma x, x, z. Here it would be
>> wrong to replace that with fadd z, fmul x, x.
>> 
>> But SimplifyFMul won't create any new instructions as far as I am
>> aware, it will only return an existing value if it could simplify the
>> fmul to it. In all those cases, we skip the multiply as part of the
>> FMA and there is no extra rounding error introduced. And I think
>> SimplifyFMulInst should only return a value if we can use it to
>> replace the FMul.
>> 
>> Does this make sense?
> 
> Not sure, what about the following cornercase:
> 
> fma z, x, y where x and y are constants chose such that the result of fmul x, y is rounded, but the interior mul in the fma is not?  SimplifyFMul should invoke the constant folder, which would round the result *appropriately for the fmul* (not the fma). Isn't that a bug?
> 
> 

That would be problematic indeed. Thanks for pointing that out. I’ll look into how to best deal with that case tomorrow.

> 
>> 
>> Cheers,
>> Florian