Page MenuHomePhabricator

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

Authored by cameron.mcinally on Oct 12 2018, 9:53 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

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

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 marked 2 inline comments as done.Oct 13 2018, 2:17 PM

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 marked an inline comment as done.

Rebase to pick up Sanjay's changes...

cameron.mcinally added a comment.EditedOct 17 2018, 12:42 PM

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?

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

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) {
  1. 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."
  1. 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?
  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();
  1. 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.

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

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.

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

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

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

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

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

@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 accepted this revision.Oct 24 2018, 6:34 AM

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.

This revision is now accepted and ready to land.Oct 24 2018, 6:34 AM
This revision was automatically updated to reflect the committed changes.