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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm 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.");
----------------
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.


================
Comment at: llvm/lib/IR/Verifier.cpp:5375-5376
+    if (VecEC.isScalable() == ResultEC.isScalable()) {
+      Assert(IdxN + ResultEC.getKnownMinValue() <= VecEC.getKnownMinValue(),
+             "experimental_vector_extract would overrun.");
+    }
----------------
Same wrapping comment as above.


================
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) {
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104468