[PATCH] D102842: [Verifier] Fail on 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:5317
            &Call);
+    Assert(IdxN % SubVecTy->getElementCount().getKnownMinValue() == 0,
+           "experimental_vector_insert index must be a constant multiple of "
----------------
I don't understand this assert and the matching one for `experimental_vector_extract`. When inserting a fixed length vector into a scalable vector we allow any index, assuming we are matching the logic of ISD::INSERT_SUBVECTOR.  If you look at SelectionDAG::getNode you see the set of asserts we use there.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1866-1867
 
-      // The result of this call is undefined if IdxN is not a constant multiple
-      // of the SubVec's minimum vector length OR the insertion overruns Vec.
-      if (IdxN % SubVecNumElts != 0 || IdxN + SubVecNumElts > VecNumElts) {
+      // The result of this call is undefined if the insertion overruns Vec.
+      if (IdxN + SubVecNumElts > VecNumElts) {
         replaceInstUsesWith(CI, UndefValue::get(CI.getType()));
----------------
I appreciate I'm late to the party here but why do we bother transforming illegal usage?  I would have thought it better to keep the intrinsic and let the verifier report an error.

By transforming the illegal usage to an undef we're just hiding a bug, which might cause people to waste time investigating a non-related area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102842