Add switch instruction to Subzero bitcode reader.

Add switch instruction to Subzero bitcode reader.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3892
R=stichnot@chromium.org

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

[Karl, 2014-09-17 19:14:17]

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

[Jim Stichnoth, 2014-09-17 20:18:21]

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

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1695:
Context->convertToIceType(Context->getTypeByID(Values[0]));
Consider defining "unsigned ValIndex = 0;" up here, then using Values[ValIndex]
here, and using Values[++ValIndex] where [1], [2], and [3] are used below.

(Comment continued on line 1721.)

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1696: if (!Ice::isScalarIntegerType(CondTy) || CondTy ==
Ice::IceType_i64) {
You can have a switch on an i64 type.

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1721: if (Values[ValIndex] != 1) {
(continuing comment from line 1695)

ValIndex can then be changed to ++ValIndex here...

(comment continued on line 1741)

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1729: if (Values[++ValIndex] != 1) {
Can this error test be combined with above?  E.g.

if (Values[ValIndex] != 1 || Values[++ValIndex] != 1) {

since the error message is the same.

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1741: ++ValIndex;
(continuing comment from line 1721)

and this ++ValIndex can be removed.

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
File tests_lit/reader_tests/switch.ll (right):

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
tests_lit/reader_tests/switch.ll:9: define i32 @testSwitch(i32 %a) {
I think it wouldn't hurt to have a test where there are no case labels except
the default.

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
tests_lit/reader_tests/switch.ll:63: define void @testSignedI32Values(i32 %a) {
Add tests for i64 as well.

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
tests_lit/reader_tests/switch.ll:114: i32 2147483647, label %exit   ; max signed
i32
For this and the I16 and I8 unsigned tests (and the TBD I64 test), I would feel
warmer and fuzzier if you also tested max_signed+1.

[Karl, 2014-09-17 22:07:20]

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

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1695:
Context->convertToIceType(Context->getTypeByID(Values[0]));
On 2014/09/17 20:18:20, stichnot wrote:
> Consider defining "unsigned ValIndex = 0;" up here, then using
Values[ValIndex]
> here, and using Values[++ValIndex] where [1], [2], and [3] are used below.
> 
> (Comment continued on line 1721.)

To try and make things clearer, I renamed ValIndex to ValCaseIndex, since it
represents the starting location of a case in the list of values. I then made
all accessors into Values (for a case) be relative
to this value.

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1696: if (!Ice::isScalarIntegerType(CondTy) || CondTy ==
Ice::IceType_i64) {
On 2014/09/17 20:18:20, stichnot wrote:
> You can have a switch on an i64 type.

Done.

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1721: if (Values[ValIndex] != 1) {
On 2014/09/17 20:18:20, stichnot wrote:
> (continuing comment from line 1695)
> 
> ValIndex can then be changed to ++ValIndex here...
> 
> (comment continued on line 1741)

See comment above.

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1729: if (Values[++ValIndex] != 1) {
On 2014/09/17 20:18:20, stichnot wrote:
> Can this error test be combined with above?  E.g.
> 
> if (Values[ValIndex] != 1 || Values[++ValIndex] != 1) {
> 
> since the error message is the same.

Done.

https://codereview.chromium.org/576243002/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1741: ++ValIndex;
On 2014/09/17 20:18:20, stichnot wrote:
> (continuing comment from line 1721)
> 
> and this ++ValIndex can be removed.

Removed.

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
File tests_lit/reader_tests/switch.ll (right):

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
tests_lit/reader_tests/switch.ll:9: define i32 @testSwitch(i32 %a) {
On 2014/09/17 20:18:20, stichnot wrote:
> I think it wouldn't hurt to have a test where there are no case labels except
> the default.

Done.

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
tests_lit/reader_tests/switch.ll:63: define void @testSignedI32Values(i32 %a) {
On 2014/09/17 20:18:20, stichnot wrote:
> Add tests for i64 as well.

Done.

https://codereview.chromium.org/576243002/diff/20001/tests_lit/reader_tests/s...
tests_lit/reader_tests/switch.ll:114: i32 2147483647, label %exit   ; max signed
i32
On 2014/09/17 20:18:20, stichnot wrote:
> For this and the I16 and I8 unsigned tests (and the TBD I64 test), I would
feel
> warmer and fuzzier if you also tested max_signed+1.

Done.

[Jim Stichnoth, 2014-09-17 22:26:34]

lgtm

[Karl, 2014-09-17 22:38:25]

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