Subzero WASM: avoid needless comparisons, add bounds check flag option

Subzero WASM: avoid needless comparisons, add bounds check flag option

Introduces a new BooleanVariable type which represents zero-extended
variables generated from an i1, saving a pointer to the original
i1. The Wasm frontend uses this to avoid comparing against 0 if
possible when translating branches. This led to about a 12%
improvement on the bzip2 spec benchmark.

This change also adds the -wasm-disable-bounds-check command line
option which omits bounds checking code.

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

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

[Eric Holk, 2016-05-06 20:10:10]

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

[Jim Stichnoth, 2016-05-06 20:37:52]

https://codereview.chromium.org/1961583002/diff/20001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1961583002/diff/20001/src/IceClFlags.def#newc...
src/IceClFlags.def:350: X(WasmNoBoundsCheck, bool, dev_opt_flag,
"wasm-disable-bounds-check",        \
Minor nit/request.  Can we call this "-wasm-bounds-check" with cl::init(true)? 
This sets up for the future experiment I proposed where you can run with
"-wasm-bounds-check=10" and have it insert 10 copies of bounds checks to see the
performance impact.

https://codereview.chromium.org/1961583002/diff/20001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1961583002/diff/20001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:396: auto *Dest = isComparison(Opcode)
I'm surprised "auto" works here, given "bool ? BooleanVariable* : Variable*".

Maybe make it Variable instead?

Also, I think it might be cleaner to use two variables:
  Variable *Dest;
  BooleanVariable *BoolDest;
and avoid all the llvm::cast<BooleanVariable> below.

https://codereview.chromium.org/1961583002/diff/20001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1388: !llvm::dyn_cast<ConstantInteger32>(Base)) {
Use isa<> instead of !dyn_cast<> here.

[Eric Holk, 2016-05-06 20:56:21]

https://codereview.chromium.org/1961583002/diff/20001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1961583002/diff/20001/src/IceClFlags.def#newc...
src/IceClFlags.def:350: X(WasmNoBoundsCheck, bool, dev_opt_flag,
"wasm-disable-bounds-check",        \
On 2016/05/06 20:37:51, stichnot wrote:
> Minor nit/request.  Can we call this "-wasm-bounds-check" with cl::init(true)?

> This sets up for the future experiment I proposed where you can run with
> "-wasm-bounds-check=10" and have it insert 10 copies of bounds checks to see
the
> performance impact.

Done.

https://codereview.chromium.org/1961583002/diff/20001/src/WasmTranslator.cpp
File src/WasmTranslator.cpp (right):

https://codereview.chromium.org/1961583002/diff/20001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:396: auto *Dest = isComparison(Opcode)
On 2016/05/06 20:37:51, stichnot wrote:
> I'm surprised "auto" works here, given "bool ? BooleanVariable* : Variable*".
> 
> Maybe make it Variable instead?
> 
> Also, I think it might be cleaner to use two variables:
>   Variable *Dest;
>   BooleanVariable *BoolDest;
> and avoid all the llvm::cast<BooleanVariable> below.

Now that you mention it, I guess I'm a little surprised too.

That said, I like having separate variables better. Dynamic casts make me
nervous.

Done.

https://codereview.chromium.org/1961583002/diff/20001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:1388: !llvm::dyn_cast<ConstantInteger32>(Base)) {
On 2016/05/06 20:37:51, stichnot wrote:
> Use isa<> instead of !dyn_cast<> here.

Done.

[Jim Stichnoth, 2016-05-06 21:09:17]

lgtm

https://codereview.chromium.org/1961583002/diff/40001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1961583002/diff/40001/src/IceClFlags.def#newc...
src/IceClFlags.def:351: cl::desc("Omit bounds checking code in WASM frontend"), 
                  \
s/Omit/Add/

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

https://codereview.chromium.org/1961583002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:397: Ice::Variable *Dest = nullptr;
s/Ice::Variable/Variable/

https://codereview.chromium.org/1961583002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:978: if (!CondBool) {
if (CondBool == nullptr)

[Eric Holk, 2016-05-06 21:28:07]

Description was changed from

==========
Subzero WASM: avoid needless comparisons, add bounds check flag option

Introduces a new BooleanVariable type which represents zero-extended
variables generated from an i1, saving a pointer to the original
i1. The Wasm frontend uses this to avoid comparing against 0 if
possible when translating branches. This led to about a 12%
improvement on the bzip2 spec benchmark.

This change also adds the -wasm-disable-bounds-check command line
option which omits bounds checking code.

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

to

==========
Subzero WASM: avoid needless comparisons, add bounds check flag option

Introduces a new BooleanVariable type which represents zero-extended
variables generated from an i1, saving a pointer to the original
i1. The Wasm frontend uses this to avoid comparing against 0 if
possible when translating branches. This led to about a 12%
improvement on the bzip2 spec benchmark.

This change also adds the -wasm-disable-bounds-check command line
option which omits bounds checking code.

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

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

[Eric Holk, 2016-05-06 21:28:08]

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

[Eric Holk, 2016-05-06 21:28:15]

https://codereview.chromium.org/1961583002/diff/40001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1961583002/diff/40001/src/IceClFlags.def#newc...
src/IceClFlags.def:351: cl::desc("Omit bounds checking code in WASM frontend"), 
                  \
On 2016/05/06 21:09:17, stichnot wrote:
> s/Omit/Add/

Done.

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

https://codereview.chromium.org/1961583002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:397: Ice::Variable *Dest = nullptr;
On 2016/05/06 21:09:17, stichnot wrote:
> s/Ice::Variable/Variable/

Done. I had to add `using Variable = Ice::Variable` to the class.

https://codereview.chromium.org/1961583002/diff/40001/src/WasmTranslator.cpp#...
src/WasmTranslator.cpp:978: if (!CondBool) {
On 2016/05/06 21:09:17, stichnot wrote:
> if (CondBool == nullptr)

Done.