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

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:5345-5348
+      Assert(
+          IdxN + SubVecEC.getKnownMinValue() <= VecEC.getKnownMinValue(),
+          "subvector operand of experimental_vector_insert would overrun the "
+          "vector being inserted into.");
----------------
paulwalker-arm wrote:
> 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.
Ahh excellent spot, thanks. 😄 


================
Comment at: llvm/test/Verifier/insert-extract-intrinsics-invalid.ll:59
+; two. Therefore, this function should not raise verifier errors.
+; CHECK-NOT: subvector operand of experimental_vector_insert would overrun the vector being inserted into.
+define <vscale x 8 x i32> @insert_overrun_scalable_fixed(<vscale x 8 x i32> %vec, <3 x i32> %subvec) {
----------------
paulwalker-arm wrote:
> 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 :)
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104468/new/

https://reviews.llvm.org/D104468