[FPEnv] PatternMatcher support for checking FNEG ignoring signed zeros

[cameron.mcinally]

Summary

@spatel

This is the first patch in a series to replace BinaryOperator::isFNeg(...) with more general pattern matcher calls.

We'll need a mechanism in the pattern matcher to match FNeg patterns that ignore signed zeros. I propose adding a new pattern matcher function, called m_ISZFNeg(...). This patch is one possible solution, but I'll leave it open for discussion. I'll also add that I have not worked on the pattern matcher before, so please review thoroughly.

There are a handful of tests that exercise the modified xform in test/Transforms/InstSimplify/fast-math.ll. So, I did not add new tests.

[cameron.mcinally]

lib/Analysis/InstructionSimplify.cpp

4512

Quick note:

I was torn here about adding a second pointer temp for Op1's captured element. But, I believe that C++'s sequence points guarantee that the reuse of X is safe.

[cameron.mcinally]

Small update to remove superfluous parentheses.

[spatel]

include/llvm/IR/PatternMatch.h

672

Let's use the existing LLVM vocabulary for "ignore signed zero":
"m_FNegNSZ"

lib/Analysis/InstructionSimplify.cpp

4510–4513

Yes, this works like you're hoping. We use this code pattern all over instsimplify/instcombine.

But we have a matcher API that will shorten this:

if (match(Op0, m_FNegNSZ(m_Specific(Op1) || 
    match(Op1, m_FNegNSZ(m_Specific(Op0))))

[spatel]

This patch is not 'NFC'. I added a test that should show an improvement at rL343936.

[cameron.mcinally]

include/llvm/IR/PatternMatch.h

672

Thanks, I didn't know that existed.

Do you want "NSZ" to be a suffix? I noticed that the others, like NSW, are prefixed: m_NSWAdd(...).

There are subtle differences between those, so I don't have a strong opinion either way.

[spatel]

include/llvm/IR/PatternMatch.h

672

Ah, I didn't notice that difference.

The suffixed version looks less like alphabet soup to me, corresponds to the order you'd see that string in IR, and corresponds to the "BinaryOperator::CreateFNegFMF()" format (bias: I added that).

So yes, my vote is m_FNegNSZ. Unless there's some reason to keep the existing names for the overflowing matchers, I'd change all of them as an NFC formatting commit for consistency.

[cameron.mcinally]

Update 'ignore signed zero' acronym to "NSZ".

I decided against the suffix change we discussed, @spatel. It appears that the IRBuilder also uses the prefix format for "NSW" and similar. Any objections to this? There are subtle differences here, so I could be persuaded otherwise.

Also update the CHECKS for a newly added test (r343936).

[lebedev.ri]

In D52934#1258977, @cameron.mcinally wrote:

Update 'ignore signed zero' acronym to "NSZ".

I decided against the suffix change we discussed, @spatel. It appears that the IRBuilder also uses the prefix format for "NSW" and similar. Any objections to this? There are subtle differences here, so I could be persuaded otherwise.

Also update the CHECKS for a newly added test (r343936).

NSW is a flag *on an instruction*. It's not a 'fneg operation of NSZ instruction'.
Suffix variants are cleaner IMO. Would be great to stick to them, and migrate the rest.

[cameron.mcinally]

In D52934#1258981, @lebedev.ri wrote:

NSW is a flag *on an instruction*. It's not a 'fneg operation of NSZ instruction'.
Suffix variants are cleaner IMO. Would be great to stick to them, and migrate the rest.

I don't feel strongly either way, but will point out one inconsistency...

The only suffixes in IRBuilder are on CreateFxxxFMF(...). Those are instructions that copy over FastMath flags. That sounds like it falls into your "is a flag on an instruction" bucket and should have a prefix instead of a suffix. But again, there are subtle differences there. For example, CreateFxxxFMF(...) takes the flags as an input and CreateNSWyyy(...) has a fixed value for the flag.

Anyone else feel strongly about the prefix/suffix decision? I don't work in this part of LLVM enough to have a strong opinion.

[cameron.mcinally]

Move "NSZ" to be a suffix.

[spatel]

LGTM

[cameron.mcinally]

Bah, I forgot the "Differential Revision:" tag. This was committed with:

---

r344084 | mcinally | 2018-10-09 16:48:00 -0500 (Tue, 09 Oct 2018) | 5 lines

[FPEnv] PatternMatcher support for checking FNEG ignoring signed zeros

https://reviews.llvm.org/D52934

Apologies for that...

include/llvm/IR/PatternMatch.h

 
 /// 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);
 }
 
+/// 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);
+}
+
 template <typename LHS, typename RHS>
 inline BinaryOp_match<LHS, RHS, Instruction::Mul> m_Mul(const LHS &L,
                                                         const RHS &R) {
   return BinaryOp_match<LHS, RHS, Instruction::Mul>(L, R);
 }
 
 template <typename LHS, typename RHS>
 inline BinaryOp_match<LHS, RHS, Instruction::FMul> m_FMul(const LHS &L,

lib/Analysis/InstructionSimplify.cpp

if (FMF.noNaNs()) {
 
     // (X * Y) / Y --> X if we can reassociate to the above form.
     Value *X;
     if (FMF.allowReassoc() && match(Op0, m_c_FMul(m_Value(X), m_Specific(Op1))))
       return X;
 
     // -X /  X -> -1.0 and
     //  X / -X -> -1.0 are legal when NaNs are ignored.
     // We can ignore signed zeros because +-0.0/+-0.0 is NaN and ignored.
-    if ((BinaryOperator::isFNeg(Op0, /*IgnoreZeroSign=*/true) &&
-         BinaryOperator::getFNegArgument(Op0) == Op1) ||
-        (BinaryOperator::isFNeg(Op1, /*IgnoreZeroSign=*/true) &&
-         BinaryOperator::getFNegArgument(Op1) == Op0))
+    if (match(Op0, m_FNegNSZ(m_Specific(Op1))) ||
+        match(Op1, m_FNegNSZ(m_Specific(Op0))))
       return ConstantFP::get(Op0->getType(), -1.0);
   }
 
   return nullptr;
 }
 
 Value *llvm::SimplifyFDivInst(Value *Op0, Value *Op1, FastMathFlags FMF,
                               const SimplifyQuery &Q) {
   return ::SimplifyFDivInst(Op0, Op1, FMF, Q, RecursionLimit);

test/Transforms/InstSimplify/fast-math.ll

 ;
   %neg = fsub float 0.000000e+00, %f
   %div = fdiv nnan float %f, %neg
   ret float %div
 }
 
 define <2 x float> @fdiv_neg_vec_undef_elt(<2 x float> %f) {
 ; CHECK-LABEL: @fdiv_neg_vec_undef_elt(
-; CHECK-NEXT:    [[NEG:%.*]] = fsub <2 x float> <float 0.000000e+00, float undef>, [[F:%.*]]
-; CHECK-NEXT:    [[DIV:%.*]] = fdiv nnan <2 x float> [[F]], [[NEG]]
-; CHECK-NEXT:    ret <2 x float> [[DIV]]
+; CHECK-NEXT:    ret <2 x float> <float -1.000000e+00, float -1.000000e+00>
 ;
   %neg = fsub <2 x float> <float 0.0, float undef>, %f
   %div = fdiv nnan <2 x float> %f, %neg
   ret <2 x float> %div
 }
 
 ; PR21126: http://llvm.org/bugs/show_bug.cgi?id=21126
 ; With loose math, sqrt(X) * sqrt(X) is just X.