Fix Subzero binary instruction to allow optional flags argument.

Fix Subzero binary instruction to allow optional flags argument.

This CL fixes pnacl-sz to handle smoothnacl.pexe.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4364
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-03-24 17:33:00]

Description was changed from

==========
Fix Subzero binary instruction to allow optional flags argument.

This CL fixes pnacl-sz to handle smoothnacl.pexe.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4364
==========

to

==========
Fix Subzero binary instruction to allow optional flags argument.

This CL fixes pnacl-sz to handle smoothnacl.pexe.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4364
==========

[Karl, 2016-03-24 17:33:00]

kschimpf@google.com changed reviewers:
+ eholk@chromium.org, jpp@chromium.org, sehr@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2016-03-24 17:46:32]

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

https://codereview.chromium.org/1831043002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2202: // Note: Old bitcode files may have an additional
'flags" operand, and
s/and/which/

and maybe reflow the comment

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
File unittest/IceParseInstsTest.cpp (right):

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
unittest/IceParseInstsTest.cpp:404: // Test that we handle the optional FLAGs
argument to binary instructions.
Would it be possible to (in addition or instead) have a lit test, in the style
of the parse_errs *.tbc files?

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
unittest/IceParseInstsTest.cpp:429: // Show bitcode objdump for BitcodeRecorsd.
BitcodeRecords

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
unittest/IceParseInstsTest.cpp:456: const uint64_t ReplaceIndex = 10;
These could be constexpr instead of const... but then you'd probably want to
make the same change to all the ReplaceIndex above for consistency... so your
call.

[John, 2016-03-24 17:49:46]

lgtm

[Karl, 2016-03-24 19:28:36]

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

https://codereview.chromium.org/1831043002/diff/20001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2202: // Note: Old bitcode files may have an additional
'flags" operand, and
On 2016/03/24 17:46:32, stichnot wrote:
> s/and/which/
> 
> and maybe reflow the comment

Done.

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
File unittest/IceParseInstsTest.cpp (right):

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
unittest/IceParseInstsTest.cpp:404: // Test that we handle the optional FLAGs
argument to binary instructions.
On 2016/03/24 17:46:32, stichnot wrote:
> Would it be possible to (in addition or instead) have a lit test, in the style
> of the parse_errs *.tbc files?

Replacing with tbc test.

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
unittest/IceParseInstsTest.cpp:429: // Show bitcode objdump for BitcodeRecorsd.
On 2016/03/24 17:46:32, stichnot wrote:
> BitcodeRecords

Ignoring, replacing with tbc test

https://codereview.chromium.org/1831043002/diff/20001/unittest/IceParseInstsT...
unittest/IceParseInstsTest.cpp:456: const uint64_t ReplaceIndex = 10;
On 2016/03/24 17:46:32, stichnot wrote:
> These could be constexpr instead of const... but then you'd probably want to
> make the same change to all the ReplaceIndex above for consistency... so your
> call.

Ignoring, replacing with tbc test.

[Jim Stichnoth, 2016-03-24 20:53:59]

lgtm

https://codereview.chromium.org/1831043002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1831043002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2201: // BINOP: [opval, opval, opcode, [flags]] Note:
Old bitcode files may have
Maybe "Note:" should start on a new line?

https://codereview.chromium.org/1831043002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2202: // an additional 'flags" operand, which is
ignored.
'flags' or "flags"

https://codereview.chromium.org/1831043002/diff/40001/tests_lit/reader_tests/...
File tests_lit/reader_tests/binop-forms.test (right):

https://codereview.chromium.org/1831043002/diff/40001/tests_lit/reader_tests/...
tests_lit/reader_tests/binop-forms.test:5: ; Checks that new form of binary add
(without flags argument) works.
"that the new" ?
(similar below)

https://codereview.chromium.org/1831043002/diff/40001/tests_lit/reader_tests/...
tests_lit/reader_tests/binop-forms.test:15: ; CHECK: define internal i32
@Function(i32 %__0, i32 %__1) { # Function(i=2:b=1)
Remove everything from "#" to the end of the line.

I don't promise that that output format is stable (which is why I put it at the
end of the line).

[Karl, 2016-03-24 22:54:42]

https://codereview.chromium.org/1831043002/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/1831043002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2201: // BINOP: [opval, opval, opcode, [flags]] Note:
Old bitcode files may have
On 2016/03/24 20:53:58, stichnot wrote:
> Maybe "Note:" should start on a new line?

Done.

https://codereview.chromium.org/1831043002/diff/40001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:2202: // an additional 'flags" operand, which is
ignored.
On 2016/03/24 20:53:58, stichnot wrote:
> 'flags' or "flags"

Done.

https://codereview.chromium.org/1831043002/diff/40001/tests_lit/reader_tests/...
File tests_lit/reader_tests/binop-forms.test (right):

https://codereview.chromium.org/1831043002/diff/40001/tests_lit/reader_tests/...
tests_lit/reader_tests/binop-forms.test:5: ; Checks that new form of binary add
(without flags argument) works.
On 2016/03/24 20:53:58, stichnot wrote:
> "that the new" ?
> (similar below)

Done.

https://codereview.chromium.org/1831043002/diff/40001/tests_lit/reader_tests/...
tests_lit/reader_tests/binop-forms.test:15: ; CHECK: define internal i32
@Function(i32 %__0, i32 %__1) { # Function(i=2:b=1)
On 2016/03/24 20:53:59, stichnot wrote:
> Remove everything from "#" to the end of the line.
> 
> I don't promise that that output format is stable (which is why I put it at
the
> end of the line).

Done.

[Karl, 2016-03-25 14:12:30]

Description was changed from

==========
Fix Subzero binary instruction to allow optional flags argument.

This CL fixes pnacl-sz to handle smoothnacl.pexe.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4364
==========

to

==========
Fix Subzero binary instruction to allow optional flags argument.

This CL fixes pnacl-sz to handle smoothnacl.pexe.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4364
R=jpp@chromium.org, stichnot@chromium.org

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

[Karl, 2016-03-25 14:12:32]

Committed patchset #4 (id:60001) manually as
e73ee859fdc010f7a7d73f91b22f594af6e69525 (presubmit successful).