This is an archive of the discontinued LLVM Phabricator instance.

Differential D96904

[IR] restrict vector reduction intrinsic types
ClosedPublic

Authored by spatel on Feb 17 2021, 2:31 PM.

Details

Reviewers
craig.topper
aemerson
dmgreen
RKSimon
bero
nikic
Commits
rG215bb15791c6: [IR] restrict vector reduction intrinsic types
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.

Diff Detail

Event Timeline

spatel created this revision.Feb 17 2021, 2:31 PM
Herald added a subscriber: mcrosier. · View Herald TranscriptFeb 17 2021, 2:31 PM
spatel requested review of this revision.Feb 17 2021, 2:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2021, 2:31 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript
nikic added a subscriber: nikic.Feb 17 2021, 2:37 PM

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

spatel added a comment.Feb 17 2021, 2:45 PM
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 updated this revision to Diff 324436.Feb 17 2021, 2:53 PM

Patch updated:
Added tests with scalar argument.

nikic added a comment.Feb 17 2021, 3:24 PM

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 added a subscriber: fhahn.Feb 18 2021, 1:17 AM
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 added a comment.Feb 18 2021, 6:00 AM
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 updated this revision to Diff 324608.Feb 18 2021, 6:04 AM
spatel edited the summary of this revision. (Show Details)

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.

Herald added subscribers: dexonsmith, hiraditya. · View Herald TranscriptFeb 18 2021, 6:04 AM
RKSimon added a comment.Feb 18 2021, 6:41 AM

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

llvm/lib/IR/Verifier.cpp
5047 ↗(On Diff #324608)

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

spatel mentioned this in rGa1e5388a7ca1: [Verifier] add tests for vector reductions; NFC.Feb 18 2021, 7:35 AM
spatel updated this revision to Diff 324635.Feb 18 2021, 7:51 AM

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 added a comment.Feb 18 2021, 7:54 AM
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.

spatel mentioned this in D97047: [Analysis][LoopVectorize] do not try to form reductions of pointers.Feb 19 2021, 6:26 AM
nikic accepted this revision.Feb 19 2021, 7:02 AM

LGTM

llvm/lib/IR/Verifier.cpp
5064 ↗(On Diff #324635)

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

This revision is now accepted and ready to land.Feb 19 2021, 7:02 AM
spatel marked an inline comment as done.Feb 19 2021, 9:23 AM
spatel added inline comments.
llvm/lib/IR/Verifier.cpp
5064 ↗(On Diff #324635)

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.

spatel mentioned this in rG5b250a27ec78: [Analysis][LoopVectorize] do not form reductions of pointers.Feb 19 2021, 11:04 AM
spatel mentioned this in rG38730b0029a4: [Verifier] add tests for saturating intrinsics; NFC.Feb 19 2021, 11:59 AM
Closed by commit rG215bb15791c6: [IR] restrict vector reduction intrinsic types (authored by spatel). · Explain WhyFeb 21 2021, 9:37 AM
This revision was automatically updated to reflect the committed changes.
spatel marked an inline comment as done.
spatel added a commit: rG215bb15791c6: [IR] restrict vector reduction intrinsic types.

Revision Contents

PathSize
llvm/
include/
llvm/
IR/
26 lines
test/
Verifier/
30 lines

Diff 324427

llvm/include/llvm/IR/Intrinsics.td

Show First 20 Lines • Show All 1,512 Lines • ▼ Show 20 Lines : Intrinsic<[], [llvm_anyptr_ty, llvm_i8_ty, llvm_anyint_ty, llvm_i32_ty],
ImmArg<ArgIndex<3>>]>; ImmArg<ArgIndex<3>>]>;
//===------------------------ Reduction Intrinsics ------------------------===// //===------------------------ Reduction Intrinsics ------------------------===//
// //
let IntrProperties = [IntrNoMem] in { let IntrProperties = [IntrNoMem] in {
def int_vector_reduce_fadd : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_fadd : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[LLVMVectorElementType<0>, [LLVMVectorElementType<0>,
llvm_anyvector_ty]>; llvm_anyfloat_ty]>;
def int_vector_reduce_fmul : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_fmul : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[LLVMVectorElementType<0>, [LLVMVectorElementType<0>,
llvm_anyvector_ty]>; llvm_anyfloat_ty]>;
def int_vector_reduce_add : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_add : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_mul : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_mul : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_and : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_and : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_or : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_or : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_xor : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_xor : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_smax : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_smax : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_smin : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_smin : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_umax : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_umax : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_umin : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_umin : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyint_ty]>;
def int_vector_reduce_fmax : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_fmax : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyfloat_ty]>;
def int_vector_reduce_fmin : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>], def int_vector_reduce_fmin : DefaultAttrsIntrinsic<[LLVMVectorElementType<0>],
[llvm_anyvector_ty]>; [llvm_anyfloat_ty]>;
} }
//===----- Matrix intrinsics ---------------------------------------------===// //===----- Matrix intrinsics ---------------------------------------------===//
def int_matrix_transpose def int_matrix_transpose
: DefaultAttrsIntrinsic<[llvm_anyvector_ty], : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
[LLVMMatchType<0>, llvm_i32_ty, llvm_i32_ty], [LLVMMatchType<0>, llvm_i32_ty, llvm_i32_ty],
[ IntrNoSync, IntrWillReturn, IntrNoMem, IntrSpeculatable, ImmArg<ArgIndex<1>>, [ IntrNoSync, IntrWillReturn, IntrNoMem, IntrSpeculatable, ImmArg<ArgIndex<1>>,
▲ Show 20 Lines • Show All 121 Lines • Show Last 20 Lines

llvm/test/Verifier/reduction-intrinsics.ll

  • This file was added.
; RUN: not opt -S -verify < %s 2>&1 | FileCheck %s
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 i32* @llvm.vector.reduce.fmin.v4p0i32(<4 x i32*>)