[ValueTracking] Allow select patterns to work on FP vectors

[tlively]

Summary

This CL allows constant vectors of floats to be recognized as non-NaN
and non-zero in select patterns. This change makes
matchSelectPattern more powerful generally, but was motivated
specifically because I wanted fminnan and fmaxnan to be created for
vector versions of the scalar patterns they are created for.

Tested with check-all on all targets. A testcase in the WebAssembly
backend that tests the non-nan codepath is in an upcoming CL.

[lebedev.ri]

I'm somewhat sure this kind of change needs it's own tests, outside of $backend.

[tlively]

In D52324#1241130, @lebedev.ri wrote:

I'm somewhat sure this kind of change needs it's own tests, outside of $backend.

I agree, but I couldn't figure out a way to test the non-NaN case in the style of the other ValueTracking tests. The backend has the advantage of being able to observe the ISel DAG in tests, but the ValueTracking tests cannot do so. I'm not aware of any instruction simplifications that depend on the isKnownNonNaN. That's not to say they don't exist, but I haven't been able to find one so far.

[lebedev.ri]

You can always write an unit-test, too.

[tlively]

• Add a unit test for recognizing fmin on vectors. It fails if either isKnownNonNaN or isKnownNonZero does not work for vectors.

[aheejin]

1. What happens when only some of the elements in a vector are NaN?
2. What happens we have -0.0 in a vector?

[tlively]

• Add tests checking that isKnownNonNaN and isKnownNonZero also return false correctly.

[tlively]

Needs fixes.

[tlively]

• Test cases where functions should return false
• Fix functions

[aheejin]

lib/Analysis/ValueTracking.cpp

4427 ↗

(On Diff #166784)

It looks like these are now repeating functionality of [[ https://github.com/llvm-mirror/llvm/blob/78f1b6d15e1a256882da9c59855ca29ce13156a3/lib/IR/Constants.cpp#L244-L255 | Constant::isNaN ]] and [[ https://github.com/llvm-mirror/llvm/blob/78f1b6d15e1a256882da9c59855ca29ce13156a3/lib/IR/Constants.cpp#L63-L83 | Constant::isZeroValue ]]. Can we use them instead?

unittests/Analysis/ValueTrackingTest.cpp

115 ↗

(On Diff #166784)

Could you add some comments for the tests about what each test is testing?

[tlively]

lib/Analysis/ValueTracking.cpp

4427 ↗

(On Diff #166784)

Not quite. Constant::isNaN and Constant::isZeroValue can detect whether *all* or *not all* of the vector elements are NaN or zero. Here we want to detect if *any* or *not any* of the elements are NaN or zero.

[xbolva00]

lib/Analysis/ValueTracking.cpp

4427 ↗

(On Diff #166784)

Can we extract this code to a helper function somewhere to Constant or so..?

[aheejin]

lib/Analysis/ValueTracking.cpp

4427 ↗

(On Diff #166784)

Why is the definition of non-NaN different from !isNaN()?

[tlively]

lib/Analysis/ValueTracking.cpp

4427 ↗

(On Diff #166784)

For the same "any" vs "all" reason. Known-non-nan means no lanes are NaN, which is different from !isNaN(), which means means not all lanes are NaN (but some of them might be).

[tlively]

lib/Analysis/ValueTracking.cpp

4427 ↗

(On Diff #166784)

@xbolva00 We could extract this code to Constant, but it doesn't really fit in there. There are no other methods of Constant that say whether each element of the constant do *not* have a particular property, which is what these functions are doing. I played around with instead adding methods to Constant for detecting whether any lane has a particular property, which is the negation of what we want. That refactoring subtly changed the way undef lanes are handled in select patterns, breaking some X86 tests I do not yet understand. I'm therefore inclined to say that the refactoring seems to be overall not worth the effort.

[aheejin]

Do we need to handle integer vectors too?

unittests/Analysis/ValueTrackingTest.cpp

102 ↗

(On Diff #166784)

Why can't this be determined? (1-2 line comment will be good too)

115 ↗

(On Diff #166784)

Nit: can we move these tests so they go after the scalar tests? Currently these are between SimpleFMin and SimpleFMax.

[tlively]

unittests/Analysis/ValueTrackingTest.cpp

102 ↗

(On Diff #166784)

The 0x7ff8000000000000 value is a NaN, so if the second lane behaves like fminnum while the other lanes behave like fminnan. Will add a comment.

[tlively]

• Address comments on tests

[aheejin]

unittests/Analysis/ValueTrackingTest.cpp

163 ↗

(On Diff #167037)

How is the third argument Ordered determined?

177 ↗

(On Diff #167037)

fminnum and fminnan are SelectionDAG terms, so I guess it would be better to describe in terms of SPNB_RETURNS_NAN and SPNB_RETURNS_OTHER here.

193 ↗

(On Diff #167037)

Can we have a few more test cases? I understand here you omitted very basic ones and put only a bit tricky cases that arises from the fact we are dealing with vectors, but having basic ones wouldn't hurt and help understanding. Such as, 1. ordered comparison that results in SPNB_RETURNS_OTHER 2. unordered comparison in which the right hand side is all NaNs

[tlively]

• Add test case, tweak wording

[tlively]

unittests/Analysis/ValueTrackingTest.cpp

163 ↗

(On Diff #167037)

By the comparison operator. For example, here ule is unordered.

193 ↗

(On Diff #167037)

I added a test for ordered comparison returning other, but having a vector of all NaNs currently results in SPF_UNKNOWN, SPNB_NA, because this is actually something that can be constant folded, rather than being a min or max. I also tried testing with undefs in vector lanes but the same thing happened. I'm not sure if failing to pattern match in the presence of undefs was an intentional decision, so I'm inclined not to change it.

[aheejin]

unittests/Analysis/ValueTrackingTest.cpp

163 ↗

(On Diff #167037)

I thought so first, but other existing test cases don’t seem to follow that rule. So I was wondering if it means something else.

[tlively]

unittests/Analysis/ValueTrackingTest.cpp

163 ↗

(On Diff #167037)

No, but it does default to false for integer min/max and patterns that are not min/max, regardless of the comparison mode.

[aheejin]

If this CL only handles floating point vectors and not integer vectors, I don't think that's a job of this CL to implement the integer part as well, but maybe it's better to add a line of comment that's marked as TODO saying currently in case of vectors only floating points are supported, or something. Otherwise LGTM.

[aheejin]

Oh, and maybe it's better to fix the title too: "floating point vectors"

[tlively]

In D52324#1249624, @aheejin wrote:

If this CL only handles floating point vectors and not integer vectors, I don't think that's a job of this CL to implement the integer part as well, but maybe it's better to add a line of comment that's marked as TODO saying currently in case of vectors only floating points are supported, or something. Otherwise LGTM.

Looking at the code again, I think integer vectors should already be handled ok, so no need for a TODO.

llvm/trunk/lib/Analysis/ValueTracking.cpp

 }
 
 static bool isKnownNonNaN(const Value *V, FastMathFlags FMF) {
   if (FMF.noNaNs())
     return true;
 
   if (auto *C = dyn_cast<ConstantFP>(V))
     return !C->isNaN();
+
+  if (auto *C = dyn_cast<ConstantDataVector>(V)) {
+    if (!C->getElementType()->isFloatingPointTy())
+      return false;
+    for (unsigned I = 0, E = C->getNumElements(); I < E; ++I) {
+      if (C->getElementAsAPFloat(I).isNaN())
+        return false;
+    }
+    return true;
+  }
+
   return false;
 }
 
 static bool isKnownNonZero(const Value *V) {
   if (auto *C = dyn_cast<ConstantFP>(V))
     return !C->isZero();
+
+  if (auto *C = dyn_cast<ConstantDataVector>(V)) {
+    if (!C->getElementType()->isFloatingPointTy())
+      return false;
+    for (unsigned I = 0, E = C->getNumElements(); I < E; ++I) {
+      if (C->getElementAsAPFloat(I).isZero())
+        return false;
+    }
+    return true;
+  }
+
   return false;
 }
 
 /// Match clamp pattern for float types without care about NaNs or signed zeros.
 /// Given non-min/max outer cmp/select from the clamp pattern this
 /// function recognizes if it can be substitued by a "canonical" min/max
 /// pattern.
 static SelectPatternResult matchFastFloatClamp(CmpInst::Predicate Pred,

llvm/trunk/unittests/Analysis/ValueTrackingTest.cpp

parseAssembly(
       "  %1 = fcmp nsz ole float %a, 0.0\n"
       "  %A = select i1 %1, float %a, float 0.0\n"
       "  ret float %A\n"
       "}\n");
   // But this should be, because we've ignored signed zeroes.
   expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, true});
 }
 
+TEST_F(MatchSelectPatternTest, VectorFMinNaN) {
+  parseAssembly(
+      "define <4 x float> @test(<4 x float> %a) {\n"
+      "  %1 = fcmp ule <4 x float> %a, \n"
+      "    <float 5.0, float 5.0, float 5.0, float 5.0>\n"
+      "  %A = select <4 x i1> %1, <4 x float> %a,\n"
+      "     <4 x float> <float 5.0, float 5.0, float 5.0, float 5.0>\n"
+      "  ret <4 x float> %A\n"
+      "}\n");
+  // Check that pattern matching works on vectors where each lane has the same
+  // unordered pattern.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
+}
+
+TEST_F(MatchSelectPatternTest, VectorFMinOtherOrdered) {
+  parseAssembly(
+      "define <4 x float> @test(<4 x float> %a) {\n"
+      "  %1 = fcmp ole <4 x float> %a, \n"
+      "    <float 5.0, float 5.0, float 5.0, float 5.0>\n"
+      "  %A = select <4 x i1> %1, <4 x float> %a,\n"
+      "     <4 x float> <float 5.0, float 5.0, float 5.0, float 5.0>\n"
+      "  ret <4 x float> %A\n"
+      "}\n");
+  // Check that pattern matching works on vectors where each lane has the same
+  // ordered pattern.
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_OTHER, true});
+}
+
+TEST_F(MatchSelectPatternTest, VectorNotFMinNaN) {
+  parseAssembly(
+      "define <4 x float> @test(<4 x float> %a) {\n"
+      "  %1 = fcmp ule <4 x float> %a, \n"
+      "    <float 5.0, float 0x7ff8000000000000, float 5.0, float 5.0>\n"
+      "  %A = select <4 x i1> %1, <4 x float> %a,\n"
+      "     <4 x float> <float 5.0, float 0x7ff8000000000000, float 5.0, float "
+      "5.0>\n"
+      "  ret <4 x float> %A\n"
+      "}\n");
+  // The lane that contains a NaN (0x7ff80...) behaves like a
+  // non-NaN-propagating min and the other lines behave like a NaN-propagating
+  // min, so check that neither is returned.
+  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+}
+
+TEST_F(MatchSelectPatternTest, VectorNotFMinZero) {
+  parseAssembly(
+      "define <4 x float> @test(<4 x float> %a) {\n"
+      "  %1 = fcmp ule <4 x float> %a, \n"
+      "    <float 5.0, float -0.0, float 5.0, float 5.0>\n"
+      "  %A = select <4 x i1> %1, <4 x float> %a,\n"
+      "     <4 x float> <float 5.0, float 0.0, float 5.0, float 5.0>\n"
+      "  ret <4 x float> %A\n"
+      "}\n");
+  // Always selects the second lane of %a if it is positive or negative zero, so
+  // this is stricter than a min.
+  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+}
+
 TEST_F(MatchSelectPatternTest, DoubleCastU) {
   parseAssembly(
       "define i32 @test(i8 %a, i8 %b) {\n"
       "  %1 = icmp ult i8 %a, %b\n"
       "  %2 = zext i8 %a to i32\n"
       "  %3 = zext i8 %b to i32\n"
       "  %A = select i1 %1, i32 %2, i32 %3\n"
       "  ret i32 %A\n"