[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/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()));
----------------
joechrisellis wrote:
> paulwalker-arm wrote:
> > 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.
> There is a second patch here which I think addresses this concern: https://reviews.llvm.org/D102907
Any objections to me recommending you merge this patch with D102907 and just have a single patch to fix the verification of these intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102842