Fix insert/extract element vector operations to check index is literal

Fix insert/extract element vector operations to check that literal
indices are used.

Also introduces a "error" instruction method for inserting an instruction
placeholder when an instruction is erroneous, and the type of generated
value is known.

BUG=
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=aff9fa2c45e39301b6f8ffb27ba8e0d0c588032e

[Karl, 2014-10-28 21:13:15]

kschimpf@google.com changed reviewers:
+ jvoung@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2014-10-28 22:25:55]

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1329: void DumpVectorIndexCheckValue(raw_ostream
&Stream,
dumpVectorIndexCheckValue

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1330: VectorIndexCheckValue Value) {
Can this method be const?  Or static?

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1355: VectorIndexCheckValue validateVectorIndex(const
Ice::Operand *Vec,
Const or static?

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1361: return VectorIndexNotI32;
This is kind of nitpicky, but which error should be returned if the vector index
is a variable of type i16 - NotI32 or NotConstant?  From a user's point of view,
I think NotConstant would be better.

And to continue on this nitpickiness path, I would rearrange the order in
VectorIndexCheckValue, dumpVectorIndexCheck(), and validateVectorIndex() to be:
  NotVector
  NotConstant
  NotI32
  NotInRange
  Valid
I think that would especially make this function look cleaner - flat instead of
an if nest.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1363: if (C->getValue() <= typeNumElements(VecType)) {
This should be '<', not '<=', right?

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1609: // appended.
Maybe add a note about why Ty would be void - an invalid call that has no dest
because the call is void.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1611: void appendErrorInstruction(Ice::Type Ty) {
const?

https://codereview.chromium.org/686913003/diff/20001/tests_lit/reader_tests/i...
File tests_lit/reader_tests/insertextract-err.ll (right):

https://codereview.chromium.org/686913003/diff/20001/tests_lit/reader_tests/i...
tests_lit/reader_tests/insertextract-err.ll:11: %e = extractelement <4 x i1> %v,
i32 %i
For completeness, should you also test out-of-range constant indexes?

[Karl, 2014-10-29 17:00:58]

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1329: void DumpVectorIndexCheckValue(raw_ostream
&Stream,
On 2014/10/28 22:25:54, stichnot wrote:
> dumpVectorIndexCheckValue

Done.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1330: VectorIndexCheckValue Value) {
On 2014/10/28 22:25:54, stichnot wrote:
> Can this method be const?  Or static?

Done.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1355: VectorIndexCheckValue validateVectorIndex(const
Ice::Operand *Vec,
On 2014/10/28 22:25:54, stichnot wrote:
> Const or static?

Done.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1361: return VectorIndexNotI32;
On 2014/10/28 22:25:54, stichnot wrote:
> This is kind of nitpicky, but which error should be returned if the vector
index
> is a variable of type i16 - NotI32 or NotConstant?  From a user's point of
view,
> I think NotConstant would be better.
> 
> And to continue on this nitpickiness path, I would rearrange the order in
> VectorIndexCheckValue, dumpVectorIndexCheck(), and validateVectorIndex() to
be:
>   NotVector
>   NotConstant
>   NotI32
>   NotInRange
>   Valid
> I think that would especially make this function look cleaner - flat instead
of
> an if nest.

I agree this is better. However, I would interchange NotI32 and NotInRange (you
are further away if the index is out of range, than the type).

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1363: if (C->getValue() <= typeNumElements(VecType)) {
On 2014/10/28 22:25:54, stichnot wrote:
> This should be '<', not '<=', right?

Yes.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1609: // appended.
On 2014/10/28 22:25:54, stichnot wrote:
> Maybe add a note about why Ty would be void - an invalid call that has no dest
> because the call is void.

Done.

https://codereview.chromium.org/686913003/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1611: void appendErrorInstruction(Ice::Type Ty) {
On 2014/10/28 22:25:54, stichnot wrote:
> const?

Not possible, modifies state when calling getNextInstVar.

https://codereview.chromium.org/686913003/diff/20001/tests_lit/reader_tests/i...
File tests_lit/reader_tests/insertextract-err.ll (right):

https://codereview.chromium.org/686913003/diff/20001/tests_lit/reader_tests/i...
tests_lit/reader_tests/insertextract-err.ll:11: %e = extractelement <4 x i1> %v,
i32 %i
On 2014/10/28 22:25:54, stichnot wrote:
> For completeness, should you also test out-of-range constant indexes?

Done.

[Jim Stichnoth, 2014-10-29 20:37:06]

lgtm

[Karl, 2014-10-29 21:32:19]

Committed patchset #3 (id:40001) manually as
aff9fa2c45e39301b6f8ffb27ba8e0d0c588032e (presubmit successful).