[llvm-commits] Patch: Floating point optimizations for SimplifyInstruction

[Michael Ilseman]

On Dec 7, 2012, at 9:34 PM, Duncan Sands <baldrick at free.fr> wrote:

> Hi Michael,
> 
>>>> +  // fsub 0, (fsub 0, X) ==> X
>>>> +  Value *X = 0;
>>>> +  if (match(Op0, m_Zero()) && match(Op1, m_FSub(m_Zero(), m_Value(X))))
>>>> +    return X;
>>> 
>>> Does this also work for -0.0?  With fast-math it should work with mixes
>>> of +0.0 and -0.0 too.
>> 
>> I'm still wrapping my head around all the nuances of floating-point negative zero. I'm going to try to crawl through this for my own benefit, feel free to skip straight to the punchline :)
>>   1)  x - (-x) shall retain the same sign as x when the result is 0
>>   2)  x - x shall be positive when resulting 0 (default rounding mode)
>> 
>> So, for the inner (fsub 0, X), where X can be +/-0:
>> (+0 - (-0) ==> +0 (Rule #1)
>> (+0 - (+0) ==> +0 (Rule #2)
>> (-0 - (-0) ==> +0 (Rule #2)
>> (-0 - (+0) ==> -0 (Rule #2)
>> 
>> Let's enumerate the outcomes, where e.g. the (o, i) pattern means the sign of the outer and inner zero constants. The resulting sign should be the same as x's for the transformation to be valid:
>> x | (o, i) | Result
>> + | (+, +) | +
>> - | (+, +) | +    <--- bad!
>> + | (+, -) | +
>> - | (+, -) | +    <--- bad!
>> + | (-, +) | -    <--- bad!
>> - | (-, +) | -
>> + | (-, -) | +
>> - | (-, -) | -
>> 
>> So, from this it seems that the below is the correct pattern:
>>   fsub(anyZero, fsub(negZero, X)) ==> X
> 
> Does GCC do the transform in exactly these cases too?
> 

No idea. I can look into it, but I don't have any experience with GCC.

>> 
>> And of course the inner zero can be any zero when no-signed-zeros fast-math flag is set.
> 
> ...
> 
>>>> +/// cst_fp_pred_ty - This helper class is used to match scalar and vector
>>>> +/// floating point constants that satisfy a specified predicate.
>>>> +template<typename Predicate>
>>>> +struct cst_fp_pred_ty : public Predicate {
>>>> +  template<typename ITy>
>>>> +  bool match(ITy *V) {
>>>> +    if (const ConstantFP *CFP = dyn_cast<ConstantFP>(V))
>>>> +      return this->isValue(CFP->getValueAPF());
>>>> +    // FIXME: Remove this.
>>> 
>>> Remove this?
>>> 
>> 
>> I'd be happy to. Why is that there for the other patterns? I didn't want to muck around with stuff that I didn't understand.
> 
> I just remembered the reason for this: nowadays testing ConstantDataVector
> should suffice, because if all the values are constant it must be a
> ConstantDataVector.  Except that it's not true because ConstantDataVector
> doesn't support every element type (this should be fixed some day).
> However, what you can do is get rid of all these cases and just use
> Constant::getSplatValue, since that handles all the possibilities for you.
> Can you please fix up the other getSplatValue uses in this file to also make
> use of Constant::getSplatValue.

Sure!

> 
>> 
>>>> +    if (const ConstantVector *CV = dyn_cast<ConstantVector>(V))
>>>> +      if (ConstantFP *CFP = dyn_cast_or_null<ConstantFP>(CV->getSplatValue()))
>>>> +        return this->isValue(CFP->getValueAPF());
>>>> +    if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(V))
>>>> +      if (ConstantFP *CFP = dyn_cast_or_null<ConstantFP>(CV->getSplatValue()))
>>>> +        return this->isValue(CFP->getValueAPF());
>>>> +    return false;
>>>> +  }
> 
> Ciao, Duncan.