Subzero, Wasm: Link and run torture tests; bug fixes.

Subzero, Wasm: Link and run torture tests; bug fixes.

This modifies the torture test script to actually link and run the
tests in addition to just translating them.

It includes a number of bug fixes as well, particularly in the
handling of boolean values. There is some cleanup of memory address
handling, and in many cases it can avoid generating useless address
computations.

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

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

[Eric Holk, 2016-04-15 22:09:05]

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

[Eric Holk, 2016-04-15 22:09:06]

https://codereview.chromium.org/1890283002/diff/1/fetch-torture-tests.sh
File fetch-torture-tests.sh (right):

https://codereview.chromium.org/1890283002/diff/1/fetch-torture-tests.sh#newc...
fetch-torture-tests.sh:16: tar xjf
/google/data/ro/users/ds/dschuff/emwasm-torture-out.tbz2
I got a change in to the WebAssembly waterfall to start archiving these
binaries, so we will be able to download them from a public location once that
change makes it into the build.

[John, 2016-04-18 19:15:55]

https://codereview.chromium.org/1890283002/diff/20001/Makefile.standalone
File Makefile.standalone (left):

https://codereview.chromium.org/1890283002/diff/20001/Makefile.standalone#old...
Makefile.standalone:263: # -Werror.
optional: can this be handled with

#pragma clang diagnostics push
#pragma clang diagnostics ignored "-Werror"
#pragma clang diagnostics pop

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:317: //assert(Count == InEdges.size());
why?

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:334: constexpr bool AllowPhisAnywhere = true;
Why is this needed? It seems weird to me to have a phi instruction in the middle
of a basic block...

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:595: Dest = makeVariable(IceType_i32);
i1?

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:934: auto ClampedAddr = makeVariable(IceType_i32);
auto *

Also, maybe you should use

Ice::getPointerType() instead?

[Eric Holk, 2016-04-18 20:57:25]

https://codereview.chromium.org/1890283002/diff/20001/Makefile.standalone
File Makefile.standalone (left):

https://codereview.chromium.org/1890283002/diff/20001/Makefile.standalone#old...
Makefile.standalone:263: # -Werror.
On 2016/04/18 19:15:54, John wrote:
> optional: can this be handled with
> 
> #pragma clang diagnostics push
> #pragma clang diagnostics ignored "-Werror"
> #pragma clang diagnostics pop

Done.

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:317: //assert(Count == InEdges.size());
On 2016/04/18 19:15:54, John wrote:
> why?

This is leftover from one of my ill-fated debugging adventures. I've re-enabled
the assert.

The thinking was that Phi nodes are sometimes built incrementally by the WASM
decoder, so it may not always know all of the incoming edges to start with.

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:334: constexpr bool AllowPhisAnywhere = true;
On 2016/04/18 19:15:54, John wrote:
> Why is this needed? It seems weird to me to have a phi instruction in the
middle
> of a basic block...

Another relic of a misguided debugging adventure. I went ahead and removed this
parameter, since I haven't actually found it useful.

The thinking was that the decoder may not know all the incoming edges to a node
ahead of time, so later on in the process it might have to add additional phis
to a block. Because the CfgNode class separates Phis from regular instructions,
they would still end up at the beginning. This flag simply let you add more Phis
after you'd already started adding normal instructions.

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:595: Dest = makeVariable(IceType_i32);
On 2016/04/18 19:15:54, John wrote:
> i1?

Eqz corresponds to the C expression `x == 0`. It's pretty common for code to go
on and treat this as a 32-bit integer and expect it to be either 0 or 1. This is
why all the boolean operations in this file compare into a temporary i1 but then
zero-extend it to an i32. Branches then truncate before doing the branch.

This is on my list of things to improve, because a lot of times we end up with
zext immediately followed by a truncate.

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:934: auto ClampedAddr = makeVariable(IceType_i32);
On 2016/04/18 19:15:54, John wrote:
> auto *
> 
> Also, maybe you should use
> 
> Ice::getPointerType() instead?

Done.

I switch to Ice::getPointerType here and in the rest of the function too.

[John, 2016-04-18 21:10:01]

lgtm

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1890283002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:595: Dest = makeVariable(IceType_i32);
On 2016/04/18 20:57:24, Eric Holk wrote:
> On 2016/04/18 19:15:54, John wrote:
> > i1?
> 
> Eqz corresponds to the C expression `x == 0`. It's pretty common for code to
go
> on and treat this as a 32-bit integer and expect it to be either 0 or 1. This
is
> why all the boolean operations in this file compare into a temporary i1 but
then
> zero-extend it to an i32. Branches then truncate before doing the branch.
> 
> This is on my list of things to improve, because a lot of times we end up with
> zext immediately followed by a truncate.

That (zext followed by truncates) also plague pnacl bitcodes. :)

[Eric Holk, 2016-04-18 22:08:22]

Description was changed from

==========
Subzero, Wasm: Link and run torture tests; bug fixes.

This modifies the torture test script to actually link and run the
tests in addition to just translating them.

It includes a number of bug fixes as well, particularly in the
handling of boolean values. There is some cleanup of memory address
handling, and in many cases it can avoid generating useless address
computations.

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

to

==========
Subzero, Wasm: Link and run torture tests; bug fixes.

This modifies the torture test script to actually link and run the
tests in addition to just translating them.

It includes a number of bug fixes as well, particularly in the
handling of boolean values. There is some cleanup of memory address
handling, and in many cases it can avoid generating useless address
computations.

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

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

[Eric Holk, 2016-04-18 22:08:23]

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