Add vector insert/extract instructions to Subzero bitcode reader.

Add vector insert/extract instructions to Subzero bitcode reader.

BUG= //code.google.com/p/nativeclient/issues/detail?id=3894
R=jvoung@chromium.org

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

[Karl, 2014-09-02 19:39:07]

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

[jvoung (off chromium), 2014-09-02 21:06:46]

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:914: Ice::Operand *getRelativeOperand(uint32_t Index) {
could these methods have been const?

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1215: StrBuf << "Extractlement index not i32. Found: "
<< Index;
Extractelement

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1223: // INSERTELT: [opval,opval,opval]
spaces between , opval ?

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
File tests_lit/reader_tests/vector.ll (right):

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:5: ; RUN:              | %llvm2ice -notranslate
-verbose=inst -build-on-read \
I don't remember what this % was for, but is this % before the llvm2ice
necessary anymore, after some of the lit.cfg changes?

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:10: %e = extractelement <4 x i1> %v, i32 %i
Technically, PNaCl only accepts constant indices for insert/extract so that they
are known to be in bounds.

[Jim Stichnoth, 2014-09-02 21:23:33]

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
File tests_lit/reader_tests/vector.ll (right):

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:5: ; RUN:              | %llvm2ice -notranslate
-verbose=inst -build-on-read \
On 2014/09/02 21:06:46, jvoung wrote:
> I don't remember what this % was for, but is this % before the llvm2ice
> necessary anymore, after some of the lit.cfg changes?

I think it's still necessary, as it's not part of LLVM_BIN_PATH.

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:10: %e = extractelement <4 x i1> %v, i32 %i
On 2014/09/02 21:06:46, jvoung wrote:
> Technically, PNaCl only accepts constant indices for insert/extract so that
they
> are known to be in bounds.

I don't think Karl's code supports constants yet. :)

[Karl, 2014-09-02 22:45:11]

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:914: Ice::Operand *getRelativeOperand(uint32_t Index) {
On 2014/09/02 21:06:46, jvoung wrote:
> could these methods have been const?

No. The parser needs to be able to generate an error and update internal state
in such cases.

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1215: StrBuf << "Extractlement index not i32. Found: "
<< Index;
On 2014/09/02 21:06:46, jvoung wrote:
> Extractelement

Done.

https://codereview.chromium.org/529113002/diff/1/src/PNaClTranslator.cpp#newc...
src/PNaClTranslator.cpp:1223: // INSERTELT: [opval,opval,opval]
On 2014/09/02 21:06:46, jvoung wrote:
> spaces between , opval ?

Done.

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
File tests_lit/reader_tests/vector.ll (right):

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:5: ; RUN:              | %llvm2ice -notranslate
-verbose=inst -build-on-read \
On 2014/09/02 21:23:32, stichnot wrote:
> On 2014/09/02 21:06:46, jvoung wrote:
> > I don't remember what this % was for, but is this % before the llvm2ice
> > necessary anymore, after some of the lit.cfg changes?
> 
> I think it's still necessary, as it's not part of LLVM_BIN_PATH.

I agree with Jim.

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:10: %e = extractelement <4 x i1> %v, i32 %i
On 2014/09/02 21:06:46, jvoung wrote:
> Technically, PNaCl only accepts constant indices for insert/extract so that
they
> are known to be in bounds.

Adding comments to clarify (both in code and tests).

[jvoung (off chromium), 2014-09-03 16:23:30]

otherwise LGTM

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
File tests_lit/reader_tests/vector.ll (right):

https://codereview.chromium.org/529113002/diff/1/tests_lit/reader_tests/vecto...
tests_lit/reader_tests/vector.ll:10: %e = extractelement <4 x i1> %v, i32 %i
On 2014/09/02 22:45:11, Karl wrote:
> On 2014/09/02 21:06:46, jvoung wrote:
> > Technically, PNaCl only accepts constant indices for insert/extract so that
> they
> > are known to be in bounds.
> 
> Adding comments to clarify (both in code and tests).

Ah right...

https://codereview.chromium.org/529113002/diff/20001/tests_lit/reader_tests/v...
File tests_lit/reader_tests/vector.ll (right):

https://codereview.chromium.org/529113002/diff/20001/tests_lit/reader_tests/v...
tests_lit/reader_tests/vector.ll:1: ; Tests insertelement and extractelement
vector instructions.
Should probably call the file something more specific than "vector.ll" unless
you plan on putting everything vector-related in here.

[Karl, 2014-09-03 16:46:34]

Committed patchset #2 (id:20001) manually as 71ba822 (presubmit successful).