This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Fold insertelement into undef if index is out of bounds
ClosedPublic

Authored by igor-laevsky on Nov 30 2017, 6:36 AM.

Details

Summary

This is branched from the D40390.

According to the llvm ir specification insertelement with out of bounds index is undef. This change implements this rule for the InstSimplify. It's mostly valuable as a way to prevent accidental crashes in various passes which are not expecting such indexes.

Diff Detail

Repository
rL LLVM

Event Timeline

igor-laevsky created this revision.

Add test case

spatel accepted this revision.Nov 30 2017, 7:33 AM
spatel added subscribers: zvi, arsenm.

LGTM.

Note that we should also check if the index itself is 'undef' (similar to D40231), but that may hit the problem described in that review (cc'ing @arsenm @zvi).

Please add a TODO comment for that.

This revision is now accepted and ready to land.Nov 30 2017, 7:33 AM
spatel requested changes to this revision.Dec 4 2017, 9:15 AM

As noted in D40721, we should add a constant folding check to the top of SimplifyInsertElementInst() to avoid a regression in folding an insertelement with all operands constant.
This won't fold directly to a constant for the test in pr28725.ll but rather a constant expression. I think that's ok. Instcombine will manage to fold that further when it does its own constant folding.

This revision now requires changes to proceed.Dec 4 2017, 9:15 AM
igor-laevsky edited edge metadata.

Add constant folding.
This breaks InstSimplify/pr28725.ll. However it's now folded into constant expression and instcombine simplifies it fully. Added relevant test case to the instcombine.

spatel accepted this revision.Dec 5 2017, 8:16 AM

LGTM - see inline comments for some small bits.

lib/Analysis/InstructionSimplify.cpp
3810 ↗(On Diff #125493)

Nit: period at end of sentence; same for comment below here.

3816 ↗(On Diff #125493)

Please add a 'TODO' comment about handling an undef index.

test/Transforms/InstCombine/pr28725.ll
3 ↗(On Diff #125493)

Do we really need to specify a triple and/or datalayout?

This revision is now accepted and ready to land.Dec 5 2017, 8:16 AM
This revision was automatically updated to reflect the committed changes.

Note: this was reverted due to the OpenGL mesa tests failures (rL320466)

For reference, discussion about the failure is going on in the mailing list post-commit thread for r319894:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171211/509121.html

Note: This was reintroduced in the rL320568.