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

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis marked an inline comment as done.
joechrisellis 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 "
----------------
paulwalker-arm wrote:
> 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.
Discussed offline -- repeating the conclusion here for clarity.

LangRef and the ISD node don’t make a distinction between the ‘inserting fixed into scalable’ and ‘inserting fixed into fixed’ case as far as the index constraints are concerned. As a result, we thought it might be better to avoid implementing something that was not documented as a thing that is supported.

So will leave this as-is for now. It's easy enough to relax in the future if need be.


================
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()));
----------------
paulwalker-arm wrote:
> 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.
Nope, I can do that. 👍 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102842