This is an archive of the discontinued LLVM Phabricator instance.

DAG: Avoid OOB when legalizing vector indexing
ClosedPublic

Authored by arsenm on Oct 31 2016, 5:03 PM.

Details

Summary

If a vector index is out of bounds, the result is supposed to be
undefined but is not undefined behavior. Change the legalization
for indexing the vector on the stack so that an out of bounds
index does not create an out of bounds memory access.

Diff Detail

Event Timeline

arsenm updated this revision to Diff 76506.Oct 31 2016, 5:03 PM
arsenm retitled this revision from to DAG: Avoid OOB when legalizing vector indexing.
arsenm updated this object.
arsenm added reviewers: eli.friedman, reames.
arsenm added a subscriber: llvm-commits.
RKSimon added a subscriber: RKSimon.Nov 1 2016, 6:24 AM

This approach looks right.

lib/CodeGen/SelectionDAG/TargetLowering.cpp
3714

I'm pretty sure this special-case isn't safe; just because it's a constant doesn't mean it's in range.

3726

It would be nice if we could round the number of elements up to a power of two... I guess it's not important.

arsenm added inline comments.Nov 1 2016, 11:03 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
3714

I was wondering about this, however I think it's OK. getNode for EXTRACT_VECTOR_ELT turns this into undef for out of bounds. For INSERT_VECTOR_ELT it seems to not, but I also haven't been able to come up with a testcase where this gets hit

3726

This should probably be an assert for now since I think this only happens after vector type legalization. When 3-vectors are eventually added this will need to be handled

efriedma added inline comments.Nov 1 2016, 11:13 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
3714

If getNode() is actually enforcing the restriction, fine... otherwise, we're likely to run into issues which are extremely difficult to reproduce because the constant is getting produced by splitting an integer in legalization or something like that.

arsenm added inline comments.Nov 1 2016, 11:17 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
3714

Should I fix getNode not doing this for insert_vector_elt to match instead? It seems broken to me that it would for one but not the other

efriedma added inline comments.Nov 1 2016, 11:23 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
3714

Actually, thinking about it a bit more, I would prefer not to leave this issue around as a trap if we start using getVectorElementPointer outside of insert/extract legalization, where we might not enforce a "constants are in range" rule. The check isn't doing anything useful anyway.

(Of course, fixing getNode would be fine.)

arsenm updated this revision to Diff 83853.Jan 10 2017, 12:54 PM
arsenm edited edge metadata.

Rebase

arsenm updated this revision to Diff 83854.Jan 10 2017, 12:58 PM
arsenm edited edge metadata.

Regenerate x86 changed test

efriedma accepted this revision.Jan 10 2017, 1:10 PM
efriedma added a reviewer: efriedma.

LGTM.

This revision is now accepted and ready to land.Jan 10 2017, 1:10 PM
arsenm closed this revision.Jan 10 2017, 2:13 PM

r291604