[PATCH] D53205: [FPEnv][NFCI] Convert more BinaryOperator::isFNeg(...) to m_FNeg(...)

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added a comment.

In https://reviews.llvm.org/D53205#1269202, @spatel wrote:

> 1. There are no users of that "IgnoreZeroSign" optional parameter in trunk - just delete it?
>
>   ``` -bool BinaryOperator::isFNeg(const Value *V, bool IgnoreZeroSign) { +bool BinaryOperator::isFNeg(const Value *V) {
>
>   ```


That would be okay, but the function itself can override the flag, when false, based on the NSZ fast math flag...

  if (!IgnoreZeroSign)
            IgnoreZeroSign = cast<Instruction>(V)->hasNoSignedZeros();



> 
> 
> 2. Within instcombine at least, it can't matter anyway. We always canonicalize: "fsub nsz 0.0, X --> fsub nsz -0.0, X". grep for:
> 
>   ``` // Subtraction from -0.0 is the canonical form of fneg." ```

This shows up in the Reassociate pass. Test @test9:llvm/test/Transforms/Reassociate/fast-ReassociateVector.ll shows the problem:

  %3 = fsub fast <2 x double> <double 0.000000e+00, double 0.000000e+00>, %a

I can prepare a small patch to elucidate it, if desired.

> 3. How about adding an m_NSZ() matcher? See m_Exact() for the template. Sorry for straying from the fneg goal, but we'd be better off changing all of the nsw/nuw matchers to this format too?

I don't think that solves the problem. The real problem is that BinaryOperator::isFNeg(...) wraps up the fast math flags check nicely. If we move to the PatternMatcher, then we have to explicitly check the fast math flag at the call sites. A quick example from Reassociate.cpp:

Currently:

  if (!BinaryOperator::isNot(I) && !BinaryOperator::isNeg(I) &&
      !BinaryOperator::isFNeg(I))
    ++Rank;

Would become (rough and ready example):

  if (!BinaryOperator::isNot(I) && !BinaryOperator::isNeg(I) &&
      !(I->getOpcode() == Instruction::FSub &&
        I->hasNoSignedZeros() &&
        match(I, m_FNegNSZ(m_Value()))) &&
      !(match(I, m_FNeg(m_Value()))))
    ++Rank;

It's pretty ugly, code qualitywise...


Repository:
  rL LLVM

https://reviews.llvm.org/D53205