[PATCH] D40390: [InstCombine] Don't crash on out of bounds index in the insertelement

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D40390#940459, @igor-laevsky wrote:

> Hi Sanjay, thanks for the comments.
>
> Updating InstSimplify is a reasonable thing to do, however it doesn't prevent instcombine from crashing. I believe we should do both changes.
>
> I splited checks for shift operations into separate review thread: https://reviews.llvm.org/D40649 (by the way, there is InstSimplify rule for them already)
>
> InstSimplify rule for the InsertElement is also in the separate review thread: https://reviews.llvm.org/D40650
>
> And this review thread is now about instcombine crash specifically in the insertelement instruction with out of bounds index.


Thanks for splitting things up.

You are correct that we still need an InstCombine change to avoid the crash, but it's different than what you have here currently. The problem is that we're not calling the new InstSimplify function at the start of InstCombiner::visitInsertElementInst().

It's standard practice in InstCombine to call the corresponding simplification routine for that opcode before trying other folds.

For example, the first thing we do for extractelement is:

  Instruction *InstCombiner::visitExtractElementInst(ExtractElementInst &EI) {
    if (Value *V = SimplifyExtractElementInst(EI.getVectorOperand(),
                                              EI.getIndexOperand(),
                                              SQ.getWithInstruction(&EI)))
      return replaceInstUsesWith(EI, V);

Now that we have an InstSimplify routine for the InsertElement opcode, we should call that function first. That avoids any crash potential from a bogus index value in InstCombine because we'll produce an undef and bail out. If you want to add that diff to https://reviews.llvm.org/D40650, I think that would be fine, or we can leave it here as a separate patch with test case.


https://reviews.llvm.org/D40390