This is an archive of the discontinued LLVM Phabricator instance.

[IR] restrict vector reduction intrinsic types
ClosedPublic

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

Details

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
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?

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

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.

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.

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 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.

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 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.

This revision was automatically updated to reflect the committed changes.
spatel marked an inline comment as done.