Page MenuHomePhabricator

[Verifier] Fail on overrunning and invalid indices for {insert,extract} vector intrinsics
ClosedPublic

Authored by joechrisellis on Jun 17 2021, 8:57 AM.

Details

Summary

With regards to overrunning, the langref (llvm/docs/LangRef.rst)
specifies:

(llvm.experimental.vector.insert)
Elements ``idx`` through (``idx`` + num_elements(``subvec``) - 1)
must be valid ``vec`` indices. If this condition cannot be determined
statically but is false at runtime, then the result vector is
undefined.

(llvm.experimental.vector.extract)
Elements ``idx`` through (``idx`` + num_elements(result_type) - 1)
must be valid vector indices. If this condition cannot be determined
statically but is false at runtime, then the result vector is
undefined.

For the non-mixed cases (e.g. inserting/extracting a scalable into/from
another scalable, or inserting/extracting a fixed into/from another
fixed), it is possible to statically check whether or not the above
conditions are met. This was previously missing from the verifier, and
if the conditions were found to be false, the result of the
insertion/extraction would be replaced with an undef.

With regards to invalid indices, the langref (llvm/docs/LangRef.rst)
specifies:

(llvm.experimental.vector.insert)
``idx`` represents the starting element number at which ``subvec``
will be inserted. ``idx`` must be a constant multiple of
``subvec``'s known minimum vector length.

(llvm.experimental.vector.extract)
The ``idx`` specifies the starting element number within ``vec``
from which a subvector is extracted. ``idx`` must be a constant
multiple of the known-minimum vector length of the result type.

Similarly, these conditions were not previously enforced in the
verifier. In some circumstances, invalid indices were permitted
silently, and in other circumstances, an undef was spawned where a
verifier error would have been preferred.

This commit adds verifier checks to enforce the constraints above.

Diff Detail

Event Timeline

joechrisellis created this revision.Jun 17 2021, 8:57 AM
joechrisellis requested review of this revision.Jun 17 2021, 8:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2021, 8:57 AM
Matt added a subscriber: Matt.Jun 17 2021, 9:49 AM
paulwalker-arm added inline comments.Jun 18 2021, 6:26 AM
llvm/lib/IR/Verifier.cpp
5341–5344

I guess if we want to be paranoid we should also Assert IdxN < VecEC.getKnownMinValue() as with a sufficiently large IdxN is could wrap and this Assert wouldn't spot it.

5372–5373

Same wrapping comment as above.

llvm/test/Verifier/insert-extract-intrinsics-invalid.ll
60

This is quite fragile as somebody might make a minor edit to the text and then the test will no longer be able to spot the thing that shouldn't happen hasn't happened. It's generally better for CHECK lines to be very specific but CHECK-NOT lines be as minimal as possible. Given all the related Asserts use experimental_vector_insert I recommend just doing:

CHECK-NOT experimental_vector_insert

Another caveat here is that when using CHECK-NOT it's possible for later tests to legitimately have this output and then the test fails. For this reason it's better to anchor the tests when possible, for example have CHECK lines that capture the function name. If this is not possible then fair enough, but it's worth checking, if you pardon the pun :)

joechrisellis marked 2 inline comments as done.

Address review comments.

  • @paulwalker-arm:
    • extend assertions to account for the possibility of integer overflow with a very large index.
    • make insert-extract-intrinsics-invalid.ll a bit more robust.
llvm/lib/IR/Verifier.cpp
5341–5344

Ahh excellent spot, thanks. 😄

llvm/test/Verifier/insert-extract-intrinsics-invalid.ll
60

This is quite fragile as somebody might make a minor edit to the text and then the test will no longer be able to spot the thing that shouldn't happen hasn't happened. It's generally better for CHECK lines to be very specific but CHECK-NOT lines be as minimal as possible.

Good point! Fixed.

Another caveat here is that when using CHECK-NOT it's possible for later tests to legitimately have this output and then the test fails. For this reason it's better to anchor the tests when possible, for example have CHECK lines that capture the function name.

Very good point, although it doesn't seem to be possible in this case. I've had a gander at the other verifier checks and none of them seem to do any kind of anchoring.

paulwalker-arm added inline comments.Jun 18 2021, 9:37 AM
llvm/lib/IR/Verifier.cpp
5372–5373

This also needs IdxN < VecEC.getKnownMinValue.

Address review comments.

  • @paulwalker-arm:
    • add missing IdxN < VecEC.getKnownMinValue for the extract case.
paulwalker-arm accepted this revision.Jun 21 2021, 3:24 AM
This revision is now accepted and ready to land.Jun 21 2021, 3:24 AM