Disable Subzero IR generation for performance testing.

Disable Subzero IR generation for performance testing.

This CL allows one to time Subzero's bitcode parsing without IR generation
(other than types, function declarations, and uninitialized global variables)
for performance testing.

BUG=None
R=jvoung@chromium.org

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

[Karl, 2014-11-03 23:32:52]

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

[Jim Stichnoth, 2014-11-04 00:07:07]

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

https://codereview.chromium.org/696383004/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:204: bool disableIRGeneration() const {
Add comment for consistency with other methods?

More importantly, to me, the method name implies that calling it will have the
effect of disabling IR generation.  How about a name like isIRGenerationEnabled
(or isIRGenerationDisabled if you don't prefer not avoiding non-double
negatives)?

https://codereview.chromium.org/696383004/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:291: if (disableIRGeneration()) {
There's a vast number of disableIRGeneration() calls, so I have no idea if
you've caught them all.

How about adding an assert to the relevant ICE generation routines?

Asserting in the Cfg ctor would prevent CfgNode, Inst, and Variable from being
allocated.

For Constant, change GlobalContext where they are managed.  One possibility is
for the GlobalContext ctor to make ConstPool=NULL, and then any attempts to
access an ICE constant would segfault.  A gentler approach would be to assert in
TypePool::getOrAdd(), and maybe also UndefPool::getOrAdd() for completeness.

[Karl, 2014-11-04 20:37:22]

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

https://codereview.chromium.org/696383004/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:204: bool disableIRGeneration() const {
On 2014/11/04 00:07:07, stichnot wrote:
> Add comment for consistency with other methods?
> 
> More importantly, to me, the method name implies that calling it will have the
> effect of disabling IR generation.  How about a name like
isIRGenerationEnabled
> (or isIRGenerationDisabled if you don't prefer not avoiding non-double
> negatives)?

Done.

https://codereview.chromium.org/696383004/diff/20001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:291: if (disableIRGeneration()) {
On 2014/11/04 00:07:07, stichnot wrote:
> There's a vast number of disableIRGeneration() calls, so I have no idea if
> you've caught them all.
> 
> How about adding an assert to the relevant ICE generation routines?
> 
> Asserting in the Cfg ctor would prevent CfgNode, Inst, and Variable from being
> allocated.
> 
> For Constant, change GlobalContext where they are managed.  One possibility is
> for the GlobalContext ctor to make ConstPool=NULL, and then any attempts to
> access an ICE constant would segfault.  A gentler approach would be to assert
in
> TypePool::getOrAdd(), and maybe also UndefPool::getOrAdd() for completeness.

Done.

[jvoung (off chromium), 2014-11-04 22:30:15]

https://codereview.chromium.org/696383004/diff/40001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/696383004/diff/40001/src/IceOperand.h#newcode388
src/IceOperand.h:388: return new (Func->allocate<Variable>())
Why not have Cfg's makeVariable() also assert?

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

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1108: if (getFlags().TimeEachFunction)
please use {

}

when the body if the if-then-statement is > 1 line (only omit if it's one line)

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1273: if (isIRGenerationDisabled())
It's sometimes not clear to me when something should be done or not. In this
case, it wouldn't crash if you alllowed the assignment of
LocalOperands[LocalIndex] = Op ?

Sometimes setOperand() is called w/ a nullptr Op, and sometimes a call to
setOperand is skipped.

getOperand() doesn't bail before indexing into LocalOperands[]

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2054: return;
Should this bail *after* checking the CondTy?

For Phi it checks the type first.

In earlier cases, there are also type checks that are skipped, but there I think
it's checking the Operand's type (and the Operand is NULL)

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2244: Ice::Operand *Callee = getOperand(CalleeIndex);
nit: Maybe move getOperand() down to before the create() -- closer to where it's
used?

In almost every other instruction, there is a bail out before it ever calls
getOperand() or getRelativeOperand(). I wonder if moving this would help
simplify things a bit if getOperand doesn't need to worry about this flag?

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2451: FuncParser->setNextConstantID(nullptr);

Shouldn't there be a bailout before calling getConstantInt32/64?

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2484: if (isIRGenerationDisabled()) {
Wouldn't the earlier check before the switch() make this unreachable?

https://codereview.chromium.org/696383004/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/696383004/diff/40001/src/llvm2ice.cpp#newcode232
src/llvm2ice.cpp:232: 
I wonder if there will be an easy way to compile-out the non-asserting
DisableIRGeneration checks that do bail outs, set nullptr, etc. for a minimal
build.  Can it be done by forcing DisableIRGeneration to false?

[Jim Stichnoth, 2014-11-04 23:43:15]

https://codereview.chromium.org/696383004/diff/40001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/696383004/diff/40001/src/IceOperand.h#newcode388
src/IceOperand.h:388: return new (Func->allocate<Variable>())
On 2014/11/04 22:30:15, jvoung wrote:
> Why not have Cfg's makeVariable() also assert?

I think the assert in the Cfg ctor dominates this.

[Karl, 2014-11-05 21:13:34]

Also updated reader tests to make sure parsing still works when no IR is
generated (It found several minor mistakes).

https://codereview.chromium.org/696383004/diff/40001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/696383004/diff/40001/src/IceOperand.h#newcode388
src/IceOperand.h:388: return new (Func->allocate<Variable>())
On 2014/11/04 22:30:15, jvoung wrote:
> Why not have Cfg's makeVariable() also assert?

Since makeVariable is a member function, and the Cfg can't exist (the
constructor does an assert if we have disabled IR generation).

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

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1108: if (getFlags().TimeEachFunction)
On 2014/11/04 22:30:15, jvoung wrote:
> please use {
> 
> }
> 
> when the body if the if-then-statement is > 1 line (only omit if it's one
line)

Done.

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1273: if (isIRGenerationDisabled())
On 2014/11/04 22:30:15, jvoung wrote:
> It's sometimes not clear to me when something should be done or not. In this
> case, it wouldn't crash if you alllowed the assignment of
> LocalOperands[LocalIndex] = Op ?
> 
> Sometimes setOperand() is called w/ a nullptr Op, and sometimes a call to
> setOperand is skipped.
> 
> getOperand() doesn't bail before indexing into LocalOperands[]

First, for setOperand, it should be called the same number of times, independent
of whether IR generation is disabled. I wanted to capture as much of the parsing
overhead as possible, to get a better measure.

Rather, I want to make sure that only nullptr is assigned into LocalOperands[]
(keeping track of size).

It should be noted (due to forward references), operands can be set out of
order, and nullptr fill is used to fill in the gaps.

I also agree with the original comment that the check should not be necessary,
and I am removing.

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2054: return;
On 2014/11/04 22:30:15, jvoung wrote:
> Should this bail *after* checking the CondTy?
> 
> For Phi it checks the type first.
> 
> In earlier cases, there are also type checks that are skipped, but there I
think
> it's checking the Operand's type (and the Operand is NULL)

Yes. I intended to check as much as possible (without major code restructuring)
before bailing out, so that we capture as much of the parse time as possible.
This should include whether the operands (as indices) are defined. To this end,
I have modified the code to consistently check operand indices, and to verify
they are nullptr.

On the other hand, CondTy is not dependent on an operand being defined, and
should be checked. Fixing.

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2244: Ice::Operand *Callee = getOperand(CalleeIndex);
On 2014/11/04 22:30:15, jvoung wrote:
> nit: Maybe move getOperand() down to before the create() -- closer to where
it's
> used?
> 
> In almost every other instruction, there is a bail out before it ever calls
> getOperand() or getRelativeOperand(). I wonder if moving this would help
> simplify things a bit if getOperand doesn't need to worry about this flag?

My original thinking was that we wanted to include times for getOperand, in that
it is part of the parser rather than IR building. The problem has been to figure
out where the separation should be. I really do not want to do major
restructuring just to capture timings.

Also, for direct calls, the operand must be to a function declaration, and is
needed to determine if the instruction generates a value. This is why I have not
removed function declarations, nor this getOperand.

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2451: FuncParser->setNextConstantID(nullptr);
On 2014/11/04 22:30:15, jvoung wrote:
> 
> Shouldn't there be a bailout before calling getConstantInt32/64?

Absolutely! Don't know how this bug got introduced. Fixing.

https://codereview.chromium.org/696383004/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2484: if (isIRGenerationDisabled()) {
On 2014/11/04 22:30:15, jvoung wrote:
> Wouldn't the earlier check before the switch() make this unreachable?

Yes. removing redundant tests.

https://codereview.chromium.org/696383004/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/696383004/diff/40001/src/llvm2ice.cpp#newcode232
src/llvm2ice.cpp:232: 
On 2014/11/04 22:30:15, jvoung wrote:
> I wonder if there will be an easy way to compile-out the non-asserting
> DisableIRGeneration checks that do bail outs, set nullptr, etc. for a minimal
> build.  Can it be done by forcing DisableIRGeneration to false?

Done.

[jvoung (off chromium), 2014-11-05 23:55:08]

https://codereview.chromium.org/696383004/diff/60001/pydir/run-llvm2ice.py
File pydir/run-llvm2ice.py (right):

https://codereview.chromium.org/696383004/diff/60001/pydir/run-llvm2ice.py#ne...
pydir/run-llvm2ice.py:64: if '-noIRgen' in args.args or '--noIRgen' in
args.args:
Is it possible to do:

%if ... --command %p2i ... | %if ... --command FileCheck ...

Otherwise we'll end up with these special short circuits within the p2i as well.

nit: not much of a fan of the capitalization within the flag "noIRgen" --
personally I prefer something like "no-ir-gen" , but I don't feel too
strongly...

https://codereview.chromium.org/696383004/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/696383004/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2118: true);
Seems like this might as well pull out the APInt first before continue'ing

https://codereview.chromium.org/696383004/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2158: // Verify arguments are defined before quiting.
nit: "quitting"

[Karl, 2014-11-06 17:06:11]

https://codereview.chromium.org/696383004/diff/60001/pydir/run-llvm2ice.py
File pydir/run-llvm2ice.py (right):

https://codereview.chromium.org/696383004/diff/60001/pydir/run-llvm2ice.py#ne...
pydir/run-llvm2ice.py:64: if '-noIRgen' in args.args or '--noIRgen' in
args.args:
On 2014/11/05 23:55:08, jvoung wrote:
> Is it possible to do:
> 
> %if ... --command %p2i ... | %if ... --command FileCheck ...
> 
> Otherwise we'll end up with these special short circuits within the p2i as
well.
> 
> nit: not much of a fan of the capitalization within the flag "noIRgen" --
> personally I prefer something like "no-ir-gen" , but I don't feel too
> strongly...

Done and Done.

https://codereview.chromium.org/696383004/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/696383004/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2118: true);
On 2014/11/05 23:55:08, jvoung wrote:
> Seems like this might as well pull out the APInt first before continue'ing

Good point. Moving.

https://codereview.chromium.org/696383004/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:2158: // Verify arguments are defined before quiting.
On 2014/11/05 23:55:08, jvoung wrote:
> nit: "quitting"

Done.

[jvoung (off chromium), 2014-11-06 17:33:05]

LGTM

[Karl, 2014-11-06 17:49:32]

Committed patchset #6 (id:100001) manually as
6fcbdddb021da27376e62c32623f92b3904b6211 (presubmit successful).