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

lib/CodeGen/SelectionDAG/FastISel.cpp

 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Operator.h"
+#include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/User.h"
 #include "llvm/IR/Value.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MachineValueType.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <iterator>
 #include <utility>
 
 using namespace llvm;
+using namespace PatternMatch;
 
 #define DEBUG_TYPE "isel"
 
 // FIXME: Remove this after the feature has proven reliable.
 static cl::opt<bool> SinkLocalValues("fast-isel-sink-local-values",
                                      cl::init(true), cl::Hidden,
                                      cl::desc("Sink local values in FastISel"));
 
bool FastISel::selectOperator(const User *I, unsigned Opcode) {
   case Instruction::Add:
     return selectBinaryOp(I, ISD::ADD);
   case Instruction::FAdd:
     return selectBinaryOp(I, ISD::FADD);
   case Instruction::Sub:
     return selectBinaryOp(I, ISD::SUB);
   case Instruction::FSub:
     // FNeg is currently represented in LLVM IR as a special case of FSub.
-    if (BinaryOperator::isFNeg(I))
+    if (match(I, m_FNeg(m_Value()))) 
       return selectFNeg(I);
     return selectBinaryOp(I, ISD::FSUB);
   case Instruction::Mul:
     return selectBinaryOp(I, ISD::MUL);
   case Instruction::FMul:
     return selectBinaryOp(I, ISD::FMUL);
   case Instruction::SDiv:
     return selectBinaryOp(I, ISD::SDIV);

lib/Transforms/InstCombine/InstCombineCasts.cpp

switch (OpI->getOpcode()) {
            RHS = Builder.CreateFPTrunc(OpI->getOperand(1), RHSMinType);
         }
 
         Value *ExactResult = Builder.CreateFRemFMF(LHS, RHS, OpI);
         return CastInst::CreateFPCast(ExactResult, Ty);
       }
     }
 
+    Value *X;
     // (fptrunc (fneg x)) -> (fneg (fptrunc x))
-    if (BinaryOperator::isFNeg(OpI)) {
-      Value *InnerTrunc = Builder.CreateFPTrunc(OpI->getOperand(1), Ty);
+    if (match(OpI, m_FNeg(m_Value(X)))) {
+      Value *InnerTrunc = Builder.CreateFPTrunc(X, Ty);
       return BinaryOperator::CreateFNegFMF(InnerTrunc, OpI);
     }
   }
 
   if (auto *II = dyn_cast<IntrinsicInst>(FPT.getOperand(0))) {
     switch (II->getIntrinsicID()) {
     default: break;
     case Intrinsic::ceil: