[IR] restrict vector reduction intrinsic types

[spatel]

Summary

The arguments in all cases should be vectors of exactly one of integer or FP.

All of the tests currently pass the verifier because we check for any vector type regardless of the type of reduction.
This obviously can't work if we mix up integer and FP, and based on current LangRef text it was not intended to work for pointers either.

The pointer case from https://llvm.org/PR49215 is what led me here. That example is still going to crash, but now it crashes sooner in the LoopVectorizer, so we will need another fix there to disallow forming reductions on pointers.

[nikic]

Does this still reject the use of a non-vector argument?

[spatel]

In D96904#2569798, @nikic wrote:

Does this still reject the use of a non-vector argument?

Yes, but the error occurs on the return type because you can't get a vector element type based on the incorrect scalar arg. So we'll see:
"Intrinsic has incorrect return type!"

I'll add a couple of tests with that malformed pattern.

[spatel]

Patch updated:
Added tests with scalar argument.

[nikic]

Hm okay, this works out fine for the vector reduction intrinsics then. However, the same general problem seems to also affect a number of other intrinsics like int.vp.* and int.matrix.*, where there is not always an additional constraint that could enforce the vector type.

[fhahn]

In D96904#2569906, @nikic wrote:

Hm okay, this works out fine for the vector reduction intrinsics then. However, the same general problem seems to also affect a number of other intrinsics like int.vp.* and int.matrix.*, where there is not always an additional constraint that could enforce the vector type.

For the Matrix intrinsics there are a bunch of extra checks in the IR verifier for constraints that cannot be expressed in tablegen.

[spatel]

In D96904#2570926, @fhahn wrote:

In D96904#2569906, @nikic wrote:

Hm okay, this works out fine for the vector reduction intrinsics then. However, the same general problem seems to also affect a number of other intrinsics like int.vp.* and int.matrix.*, where there is not always an additional constraint that could enforce the vector type.

For the Matrix intrinsics there are a bunch of extra checks in the IR verifier for constraints that cannot be expressed in tablegen.

Thanks - then I'll add cases for the vector reduction intrinsics to the C++ with the extra logic too. No need to make this hacky.

[spatel]

Patch updated:
Add C++ logic and leave the tablegen as-is.
I'm not sure if we would prefer to put all checks into C++ in this situation, but it looks like other intrinsics have this split too.

[RKSimon]

What about return values? Are they checked to match the reduction vector element?

llvm/lib/IR/Verifier.cpp

5047

Match arg0 type to arg1 element type? I'm not sure the tablegen ensure that

[spatel]

Patch updated:
No code changes, but I pushed tests to check existing tablegen functionality (result type matches vector element, start argument type matches vector element, vector arg is a vector) as a preliminary step: a1e5388

So the new tests correspond to the new code only, and this patch should not interfere with existing verifier code.

[spatel]

In D96904#2571456, @RKSimon wrote:

What about return values? Are they checked to match the reduction vector element?

That's in the existing tablegen-based checking. I tried to make the old/new checking clearer by committing tests. We can move everything to C++ (or back to tablegen as with the earlier draft) if that is less confusing.

[nikic]

LGTM

llvm/lib/IR/Verifier.cpp

5020–5051

Side note: The checks for these intrinsics look unnecessary. This should already be ensured by the tablegen definitions.

[spatel]

llvm/lib/IR/Verifier.cpp

5020–5051

Those were added in stages with:
D53053
D53340
D53783
D83216

I don't see any Verifier regression tests for these. I can add some in a separate patch and clean up.

llvm/lib/IR/Verifier.cpp

if (isa<CallInst>(Call)) {
 
       if (!Call.getType()->isVoidTy() && RI)
         Assert(RI->getReturnValue() == &Call,
                "calls to experimental_deoptimize must be followed by a return "
                "of the value computed by experimental_deoptimize");
     }
 
     break;
   }
+  case Intrinsic::vector_reduce_and:
+  case Intrinsic::vector_reduce_or:
+  case Intrinsic::vector_reduce_xor:
+  case Intrinsic::vector_reduce_add:
+  case Intrinsic::vector_reduce_mul:
+  case Intrinsic::vector_reduce_smax:
+  case Intrinsic::vector_reduce_smin:
+  case Intrinsic::vector_reduce_umax:
+  case Intrinsic::vector_reduce_umin: {
+    Type *ArgTy = Call.getArgOperand(0)->getType();
+    Assert(ArgTy->isIntOrIntVectorTy() && ArgTy->isVectorTy(),
+           "Intrinsic has incorrect argument type!");
+    break;
+  }
+  case Intrinsic::vector_reduce_fmax:
+  case Intrinsic::vector_reduce_fmin: {
+    Type *ArgTy = Call.getArgOperand(0)->getType();
+    Assert(ArgTy->isFPOrFPVectorTy() && ArgTy->isVectorTy(),
+           "Intrinsic has incorrect argument type!");
+    break;
+  }
+  case Intrinsic::vector_reduce_fadd:
+  case Intrinsic::vector_reduce_fmul: {
+    // Unlike the other reductions, the first argument is a start value. The
+    // second argument is the vector to be reduced.
+    Type *ArgTy = Call.getArgOperand(1)->getType();
+    Assert(ArgTy->isFPOrFPVectorTy() && ArgTy->isVectorTy(),
+           "Intrinsic has incorrect argument type!");
+    break;
+  }
   case Intrinsic::smul_fix:
   case Intrinsic::smul_fix_sat:
   case Intrinsic::umul_fix:
   case Intrinsic::umul_fix_sat:
   case Intrinsic::sdiv_fix:
   case Intrinsic::sdiv_fix_sat:
   case Intrinsic::udiv_fix:
   case Intrinsic::udiv_fix_sat: {
     Value *Op1 = Call.getArgOperand(0);

llvm/test/Verifier/reduction-intrinsics.ll

 ; Wrong result type.
 
 define i64 @result_too_wide(<4 x i32> %x) {
 ; CHECK: Intrinsic has incorrect return type!
   %r = call i64 @llvm.vector.reduce.add.v4i32(<4 x i32> %x)
   ret i64 %r
 }
 
+; We should have the appropriate (either int or FP) type of argument
+; for any vector reduction.
+
+define float @not_float_reduce(<4 x float> %x) {
+; CHECK: Intrinsic has incorrect argument type!
+  %r = call float @llvm.vector.reduce.umin.v4f32(<4 x float> %x)
+  ret float %r
+}
+
+define i32* @not_pointer_reduce(<4 x i32*> %x) {
+; CHECK: Intrinsic has incorrect argument type!
+  %r = call i32* @llvm.vector.reduce.or.v4p0i32(<4 x i32*> %x)
+  ret i32* %r
+}
+
+define i32 @not_integer_reduce(<4 x i32> %x) {
+; CHECK: Intrinsic has incorrect argument type!
+  %r = call i32 @llvm.vector.reduce.fadd.v4i32(i32 0, <4 x i32> %x)
+  ret i32 %r
+}
+
+define i32* @not_pointer_reduce2(<4 x i32*> %x) {
+; CHECK: Intrinsic has incorrect argument type!
+  %r = call i32* @llvm.vector.reduce.fmin.v4p0i32(<4 x i32*> %x)
+  ret i32* %r
+}
+
 declare float @llvm.vector.reduce.umin.v4f32(<4 x float>)
 declare i32* @llvm.vector.reduce.or.v4p0i32(<4 x i32*>)
 declare i32 @llvm.vector.reduce.fadd.v4i32(i32, <4 x i32>)
 declare float @llvm.vector.reduce.fadd.v4f32(double, <4 x float>)
 declare i32* @llvm.vector.reduce.fmin.v4p0i32(<4 x i32*>)
 declare float @llvm.vector.reduce.fmax.f32(float)
 declare i32 @llvm.vector.reduce.smax.i32(i32)
 declare i64 @llvm.vector.reduce.add.v4i32(<4 x i32>)