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

[cameron.mcinally]

Summary

Continuing the work started in D52934...

This patch replaces more uses of BinaryOperator::isFNeg(...) with the more general m_FNeg(...).

Please excuse the small patch, I wanted to run a design decision passed @spatel before proceeding. Sanjay, please note the inline comment.

[cameron.mcinally]

lib/Transforms/InstCombine/InstCombineInternal.h

87 ↗

(On Diff #169443)

@spatel, have you given any thought to replacing the integer isNeg (and friends) BinaryOperator helper functions with pattern matcher functions? It will help keep the code more uniform. Just thinking aloud...

[spatel]

I suspect that none of these changes are actually 'NFC'.
Example:
rL344458

I realize it's tedious to come up with these tests, so it's probably ok to say we accept these kinds of changes as general goodness without regression test proof. But we are in the process of enhancing both the IR and backend:
rL343727
rL343940
...to optimize harder based on vector undefs, so any additional test coverage for those cases is much appreciated.

lib/Transforms/InstCombine/InstCombineInternal.h

87 ↗

(On Diff #169443)

Yes, we should get rid of the integer binop helpers as part of the fneg cleanup.

[cameron.mcinally]

In D53205#1264508, @spatel wrote:

I suspect that none of these changes are actually 'NFC'.

Yes, I agree with that. The test case you added for D52934 shows there was a *seemingly* positive change.

I don't have a lot of intuition built up around this code, but will add test cases where I see differences...

lib/Transforms/InstCombine/InstCombineInternal.h

87 ↗

(On Diff #169443)

It looks like you handled this in rL344458. Thanks for that.

[cameron.mcinally]

Rebase to pick up Sanjay's changes...

[cameron.mcinally]

Ah, yeah, there is a silent regression here. From BinaryOperator::isFNeg(...):

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

So that code is checking the NoSignedZero FastMath flag and then ignoring the sign on a zero if found.

We currently don't have that capability with the PatternMatcher functions. Those functions are fairly rigid too, so it won't be easy to add that capability. Also, it will be ugly to compensate for this shortcoming at the caller.

Any suggestions on how to proceed?

[cameron.mcinally]

In D53205#1267982, @cameron.mcinally wrote:

Also, it will be ugly to compensate for this shortcoming at the caller.

Modifying the callers could look like this:

if(I->hasNoSignedZeros() ?
      match(I, m_FNegNSZ(m_Value())) :
      match(I, m_FNeg(m_Value())))

It's not pretty. Also, we have to ensure that Instruction I is an FPMathOperator, so it gets uglier without context...

[spatel]

In D53205#1267982, @cameron.mcinally wrote:

Ah, yeah, there is a silent regression here. From BinaryOperator::isFNeg(...):

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

So that code is checking the NoSignedZero FastMath flag and then ignoring the sign on a zero if found.

We currently don't have that capability with the PatternMatcher functions. Those functions are fairly rigid too, so it won't be easy to add that capability. Also, it will be ugly to compensate for this shortcoming at the caller.

Any suggestions on how to proceed?

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) {

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."

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?

[cameron.mcinally]

In 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...

[cameron.mcinally]

Just thinking aloud. I really don't have enough experience with this framework to say for sure...

This hasNoSignedZeros(...) function is pretty rigid:

bool Instruction::hasNoSignedZeros() const {
  assert(isa<FPMathOperator>(this) && "getting fast-math flag on invalid op");
  return cast<FPMathOperator>(this)->hasNoSignedZeros();
}

So this will assert if the class isn't an FPMathOperator. Maybe this function (and friends) should be relaxed to return false if it's not an FPMathOperator?

That way our explicit code wouldn't be so verbose. E,g.:

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

*Note: that could probably be cleaned up more. Just a rough example.

[spatel]

In D53205#1269415, @cameron.mcinally wrote:

Just thinking aloud. I really don't have enough experience with this framework to say for sure...

This hasNoSignedZeros(...) function is pretty rigid:

bool Instruction::hasNoSignedZeros() const {
  assert(isa<FPMathOperator>(this) && "getting fast-math flag on invalid op");
  return cast<FPMathOperator>(this)->hasNoSignedZeros();
}

So this will assert if the class isn't an FPMathOperator. Maybe this function (and friends) should be relaxed to return false if it's not an FPMathOperator?

That way our explicit code wouldn't be so verbose. E,g.:

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

*Note: that could probably be cleaned up more. Just a rough example.

Ok, I may not be seeing the problem correctly, but let me try 1 more suggestion. What if we adjust the regular m_FNeg() definition to look for 'nsz' on the op itself:

Index: include/llvm/IR/PatternMatch.h
===================================================================
--- include/llvm/IR/PatternMatch.h	(revision 344898)
+++ include/llvm/IR/PatternMatch.h	(working copy)
@@ -659,11 +659,32 @@
   return BinaryOp_match<LHS, RHS, Instruction::FSub>(L, R);
 }
 
+template <typename Op_t> struct FNeg_match {
+  Op_t X;
+
+  FNeg_match(const Op_t &Op) : X(Op) {}
+  template <typename OpTy> bool match(OpTy *V) {
+    auto *FPMO = dyn_cast<FPMathOperator>(V);
+    if (!FPMO || FPMO->getOpcode() != Instruction::FSub)
+      return false;
+    if (FPMO->hasNoSignedZeros()) {
+      // With 'nsz', any zero goes.
+      if (!cstfp_pred_ty<is_any_zero_fp>().match(FPMO->getOperand(0)))
+        return false;
+    } else {
+      // Without 'nsz', we need fsub -0.0, X exactly.
+      if (!cstfp_pred_ty<is_neg_zero_fp>().match(FPMO->getOperand(0)))
+        return false;
+    }
+    return X.match(FPMO->getOperand(1));
+  }
+};
+
 /// Match 'fneg X' as 'fsub -0.0, X'.
-template <typename RHS>
-inline BinaryOp_match<cstfp_pred_ty<is_neg_zero_fp>, RHS, Instruction::FSub>
-m_FNeg(const RHS &X) {
-  return m_FSub(m_NegZeroFP(), X);
+template <typename OpTy>
+inline FNeg_match<OpTy>
+m_FNeg(const OpTy &X) {
+  return FNeg_match<OpTy>(X);
 }
 
 /// Match 'fneg X' as 'fsub +-0.0, X'.

[cameron.mcinally]

Ah, yeah. That would work. Thanks, Sanjay.

Apologies if that is what you were suggesting in the previous comment. I misunderstood the suggestion.

[spatel]

In D53205#1271492, @cameron.mcinally wrote:

Ah, yeah. That would work. Thanks, Sanjay.

Apologies if that is what you were suggesting in the previous comment. I misunderstood the suggestion.

I was just throwing out code hoping that one of those fit the problem. :)

Barring complaint/revert, the related integer neg/not code is gone after:
rL345052

With these preliminary commits:
rL345050
rL345043
rL345042
rL345041
rL345036
rL345030

[cameron.mcinally]

Rebase, add Sanjay's changes, and replace some more BinaryOperator::isFNeg(...) calls.

@spatel notice the one test change. That seems like a good change to me, i.e. replace a constant pool load with undef. But, perhaps the CP load is a canonicalization that I don't know about....

[cameron.mcinally]

In D53205#1273112, @cameron.mcinally wrote:

@spatel notice the one test change. That seems like a good change to me, i.e. replace a constant pool load with undef. But, perhaps the CP load is a canonicalization that I don't know about....

Although, the xform doesn't appear to be doing anything now. So maybe the test was there for a reason.

[spatel]

LGTM.

The changed test was looking for an infinite loop:
rL253655
And we commented that it was likely low value here:
D44258
...so I'm not worried about that diff.

llvm/trunk/include/llvm/IR/PatternMatch.h

 }
 
 template <typename LHS, typename RHS>
 inline BinaryOp_match<LHS, RHS, Instruction::FSub> m_FSub(const LHS &L,
                                                           const RHS &R) {
   return BinaryOp_match<LHS, RHS, Instruction::FSub>(L, R);
 }
 
+template <typename Op_t> struct FNeg_match {
+  Op_t X;
+
+  FNeg_match(const Op_t &Op) : X(Op) {}
+  template <typename OpTy> bool match(OpTy *V) {
+    auto *FPMO = dyn_cast<FPMathOperator>(V);
+    if (!FPMO || FPMO->getOpcode() != Instruction::FSub)
+      return false;
+    if (FPMO->hasNoSignedZeros()) {
+      // With 'nsz', any zero goes.
+      if (!cstfp_pred_ty<is_any_zero_fp>().match(FPMO->getOperand(0)))
+        return false;
+    } else {
+      // Without 'nsz', we need fsub -0.0, X exactly.
+      if (!cstfp_pred_ty<is_neg_zero_fp>().match(FPMO->getOperand(0)))
+        return false;
+    }
+    return X.match(FPMO->getOperand(1));
+  }
+};
+
 /// Match 'fneg X' as 'fsub -0.0, X'.
-template <typename RHS>
-inline BinaryOp_match<cstfp_pred_ty<is_neg_zero_fp>, RHS, Instruction::FSub>
-m_FNeg(const RHS &X) {
-  return m_FSub(m_NegZeroFP(), X);
+template <typename OpTy>
+inline FNeg_match<OpTy>
+m_FNeg(const OpTy &X) {
+  return FNeg_match<OpTy>(X);
 }
 
 /// Match 'fneg X' as 'fsub +-0.0, X'.
 template <typename RHS>
 inline BinaryOp_match<cstfp_pred_ty<is_any_zero_fp>, RHS, Instruction::FSub>
 m_FNegNSZ(const RHS &X) {
   return m_FSub(m_AnyZeroFP(), X);
 }

llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp

unsigned ReassociatePass::getRank(Value *V) {
   // cannot be loops in the value graph that do not go through PHI nodes.
   unsigned Rank = 0, MaxRank = RankMap[I->getParent()];
   for (unsigned i = 0, e = I->getNumOperands(); i != e && Rank != MaxRank; ++i)
     Rank = std::max(Rank, getRank(I->getOperand(i)));
 
   // If this is a 'not' or 'neg' instruction, do not count it for rank. This
   // assures us that X and ~X will have the same rank.
   if (!match(I, m_Not(m_Value())) && !match(I, m_Neg(m_Value())) &&
-      !BinaryOperator::isFNeg(I))
+      !match(I, m_FNeg(m_Value())))
     ++Rank;
 
   LLVM_DEBUG(dbgs() << "Calculated Rank[" << V->getName() << "] = " << Rank
                     << "\n");
 
   return ValueRankMap[I] = Rank;
 }
 
#endif
                   !cast<Instruction>(Op)->isFast())) &&
              "Should have been handled above!");
       assert(Op->hasOneUse() && "Has uses outside the expression tree!");
 
       // If this is a multiply expression, turn any internal negations into
       // multiplies by -1 so they can be reassociated.
       if (BinaryOperator *BO = dyn_cast<BinaryOperator>(Op))
         if ((Opcode == Instruction::Mul && match(BO, m_Neg(m_Value()))) ||
-            (Opcode == Instruction::FMul && BinaryOperator::isFNeg(BO))) {
+            (Opcode == Instruction::FMul && match(BO, m_FNeg(m_Value())))) {
           LLVM_DEBUG(dbgs()
                      << "MORPH LEAF: " << *Op << " (" << Weight << ") TO ");
           BO = LowerNegateToMultiply(BO);
           LLVM_DEBUG(dbgs() << *BO << '\n');
           Worklist.push_back(std::make_pair(BO, Weight));
           Changed = true;
           continue;
         }
if (BinaryOperator *I =
     // could expose more reassociating opportunities.
     ToRedo.insert(I);
     return I;
   }
 
   // Okay, we need to materialize a negated version of V with an instruction.
   // Scan the use lists of V to see if we have one already.
   for (User *U : V->users()) {
-    if (!match(U, m_Neg(m_Value())) && !BinaryOperator::isFNeg(U))
+    if (!match(U, m_Neg(m_Value())) && !match(U, m_FNeg(m_Value())))
       continue;
 
     // We found one!  Now we have to make sure that the definition dominates
     // this use.  We do this by moving it to the entry block (if it is a
     // non-instruction value) or right after the definition.  These negates will
     // be zapped by reassociate later, so we don't need much finesse here.
     BinaryOperator *TheNeg = cast<BinaryOperator>(U);
 
static Value *NegateValue(Value *V, Instruction *BI,
   BinaryOperator *NewNeg = CreateNeg(V, V->getName() + ".neg", BI, BI);
   ToRedo.insert(NewNeg);
   return NewNeg;
 }
 
 /// Return true if we should break up this subtract of X-Y into (X + -Y).
 static bool ShouldBreakUpSubtract(Instruction *Sub) {
   // If this is a negation, we can't split it up!
-  if (match(Sub, m_Neg(m_Value())) || BinaryOperator::isFNeg(Sub))
+  if (match(Sub, m_Neg(m_Value())) || match(Sub, m_FNeg(m_Value()))) 
     return false;
 
   // Don't breakup X - undef.
   if (isa<UndefValue>(Sub->getOperand(1)))
     return false;
 
   // Don't bother to break this up unless either the LHS is an associable add or
   // subtract or if this is only used by one.
if (i+1 != Ops.size() && Ops[i+1].Op == TheOp) {
       --i;
       e = Ops.size();
       continue;
     }
 
     // Check for X and -X or X and ~X in the operand list.
     Value *X;
     if (!match(TheOp, m_Neg(m_Value(X))) && !match(TheOp, m_Not(m_Value(X))) &&
-        !BinaryOperator::isFNeg(TheOp))
+        !match(TheOp, m_FNeg(m_Value(X))))
       continue;
 
-    if (BinaryOperator::isFNeg(TheOp))
-      X = BinaryOperator::getFNegArgument(TheOp);
-
     unsigned FoundX = FindInOperandList(Ops, i, X);
     if (FoundX == i)
       continue;
 
     // Remove X and -X from the operand list.
     if (Ops.size() == 2 &&
-        (match(TheOp, m_Neg(m_Value())) || BinaryOperator::isFNeg(TheOp)))
+        (match(TheOp, m_Neg(m_Value())) || match(TheOp, m_FNeg(m_Value()))))
       return Constant::getNullValue(X->getType());
 
     // Remove X and ~X from the operand list.
     if (Ops.size() == 2 && match(TheOp, m_Not(m_Value())))
       return Constant::getAllOnesValue(X->getType());
 
     Ops.erase(Ops.begin()+i);
     if (i < FoundX)
if (ShouldBreakUpSubtract(I)) {
       }
     }
   } else if (I->getOpcode() == Instruction::FSub) {
     if (ShouldBreakUpSubtract(I)) {
       Instruction *NI = BreakUpSubtract(I, RedoInsts);
       RedoInsts.insert(I);
       MadeChange = true;
       I = NI;
-    } else if (BinaryOperator::isFNeg(I)) {
+    } else if (match(I, m_FNeg(m_Value()))) {
       // Otherwise, this is a negation.  See if the operand is a multiply tree
       // and if this is not an inner node of a multiply tree.
       if (isReassociableOp(I->getOperand(1), Instruction::FMul) &&
           (!I->hasOneUse() ||
            !isReassociableOp(I->user_back(), Instruction::FMul))) {
         // If the negate was simplified, revisit the users to see if we can
         // reassociate further.
         Instruction *NI = LowerNegateToMultiply(I);

llvm/trunk/test/Transforms/Reassociate/fp-expr.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -reassociate < %s | FileCheck %s
 
 define void @test1() {
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT:    [[T1:%.*]] = tail call <4 x float> @blam()
-; CHECK-NEXT:    [[T1_NEG:%.*]] = fsub fast <4 x float> <float -0.000000e+00, float -0.000000e+00, float -0.000000e+00, float -0.000000e+00>, [[T1]]
-; CHECK-NEXT:    [[T24:%.*]] = fadd fast <4 x float> [[T1_NEG]], undef
+; CHECK-NEXT:    [[T23:%.*]] = fsub fast <4 x float> undef, [[T1]]
+; CHECK-NEXT:    [[T24:%.*]] = fadd fast <4 x float> [[T23]], undef
 ; CHECK-NEXT:    tail call void @wombat(<4 x float> [[T24]])
 ; CHECK-NEXT:    ret void
 ;
   %t1 = tail call <4 x float> @blam()
   %t23 = fsub fast <4 x float> undef, %t1
   %t24 = fadd fast <4 x float> %t23, undef
   tail call void @wombat(<4 x float> %t24)
   ret void