[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

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

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

[tlively]

lib/Analysis/ValueTracking.cpp

4427

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

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

[aheejin]

lib/Analysis/ValueTracking.cpp

4427

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

[tlively]

lib/Analysis/ValueTracking.cpp

4427

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

@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

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

115

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

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

200

How is the third argument Ordered determined?

214

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.

230

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

200

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

230

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

200

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

200

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.

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,

unittests/Analysis/ValueTrackingTest.cpp

parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ult float %a, 5.0\n"
       "  %A = select i1 %1, float %a, float 5.0\n"
       "  ret float %A\n"
       "}\n");
   expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
 }
 
+TEST_F(MatchSelectPatternTest, VectorFMin) {
+  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");
+  expectPattern({SPF_FMINNUM, SPNB_RETURNS_NAN, false});
+}
+
+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");
+  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");
+  expectPattern({SPF_UNKNOWN, SPNB_NA, false});
+}
+
 TEST_F(MatchSelectPatternTest, SimpleFMax) {
   parseAssembly(
       "define float @test(float %a) {\n"
       "  %1 = fcmp ogt float %a, 5.0\n"
       "  %A = select i1 %1, float %a, float 5.0\n"
       "  ret float %A\n"
       "}\n");
   expectPattern({SPF_FMAXNUM, SPNB_RETURNS_OTHER, true});
parseAssembly(
       "  %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"
       "}\n");
   // We should be able to look through the situation where we cast both operands
   // to the select.
   expectPattern({SPF_UMIN, SPNB_NA, false});
 }
 
 TEST_F(MatchSelectPatternTest, DoubleCastS) {
   parseAssembly(
       "define i32 @test(i8 %a, i8 %b) {\n"
       "  %1 = icmp slt i8 %a, %b\n"
       "  %2 = sext i8 %a to i32\n"
       "  %3 = sext i8 %b to i32\n"
       "  %A = select i1 %1, i32 %2, i32 %3\n"
       "  ret i32 %A\n"
       "}\n");
   // We should be able to look through the situation where we cast both operands
   // to the select.
   expectPattern({SPF_SMIN, SPNB_NA, false});
 }
 
 TEST_F(MatchSelectPatternTest, DoubleCastBad) {
   parseAssembly(
       "define i32 @test(i8 %a, i8 %b) {\n"
       "  %1 = icmp ult i8 %a, %b\n"
       "  %2 = zext i8 %a to i32\n"
       "  %3 = sext i8 %b to i32\n"
       "  %A = select i1 %1, i32 %2, i32 %3\n"
       "  ret i32 %A\n"
       "}\n");
   // The cast types here aren't the same, so we cannot match an UMIN.
   expectPattern({SPF_UNKNOWN, SPNB_NA, false});
 }
 
 TEST(ValueTracking, GuaranteedToTransferExecutionToSuccessor) {
   StringRef Assembly =
       "declare void @nounwind_readonly(i32*) nounwind readonly "
       "declare void @nounwind_argmemonly(i32*) nounwind argmemonly "
       "declare void @throws_but_readonly(i32*) readonly "
       "declare void @throws_but_argmemonly(i32*) argmemonly "
       " "
       "declare void @unknown(i32*) "
       " "