Introduction of improved switch lowering.

Introduction of improved switch lowering.

This includes the high level analysis of switches, the x86 lowering,
the repointing of targets in jump tables and ASM emission of jump
tables.

The technique uses jump tables, range test and binary search with
worst case O(lg n) which improves the previous worst case of O(n)
from a sequential search.

Use is hidden by the --adv-switch flag as the IAS emission still
needs to be implemented.

BUG=None
R=jvoung@chromium.org, stichnot@chromium.org

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

[jvoung (off chromium), 2015-07-15 15:40:32]

Could you add a .ll smoke test using the transitional flag and filetype=asm, for
a few cases? (will have to be a new test file...)

Don't have to be too specific in the CHECKS for what asm is generated, but a
test could at least show when a jumptable matches and when it doesn't.

[jvoung (off chromium), 2015-07-15 18:32:02]

Thanks!

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp#newcode218
src/IceCfgNode.cpp:218: // not comtain duplicates.
comtain -> contain

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode969
src/IceInst.cpp:969: void InstJumpTable::emitIAS(const Cfg *Func) const {
add a newline between functions

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode970
src/IceInst.cpp:970: // TODO(ascull): put jump table in the global context for
emision later
emision -> emission

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:1: //===----- subzero/src/IceSwitchLowering.h - Switch
lowering -----*- C++ -*-==//
.cpp

and no need for the -*- C++ -*- in the .cpp

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:49: // Small number of cases is trivial
By "trivial" you mean that loading from the jump table, doing indirect jumps,
and for NaCl sandboxing, aligning the CfgNode targets of the jump table, seems
to be not "profitable" compared to direct jumps (which don't require the target
to be aligned)?

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:53: // Test for a single jump table
I think it would be good to document why a single one instead of a tree of them.

For a tree of jump tables, we'd have to traverse to the leaves before anything
happens. So, we're sort of looking for an *unbalanced* tree, where there is a
bias toward frequently executed code.  The problem is that we don't have profile
information to do this *well*. So, this single jump table is a good starting
point where you can get to the jump table quickly without figuring out how to
unbalance the tree.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:81: bool CaseCluster::tryAppend(const CaseCluster& x)
{
"x" -> "X"

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:83: bool can = this->Label == x.Label && this->High+1
== x.Low;
Start variable name with capital letter ("Can")

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:37: /// Create a cluster of a single case represented by
a unitary range.
At first I was surprised this was unitary, while the jumptable version is not
unitary. Perhaps the case is that tryAppend will increase the range as you go?
Could you clarify?

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:44: private:
nit: I tend to prefer just having all the public: section continuous and the
private: continuous.

It should be okay to define the getters in terms of the fields before the fields
are actually defined?

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:72: bool tryAppend(const CaseCluster& x);
I think LLVM style puts the & next to the x? Did clang-format get this?

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Base.h
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:210: /// set, if in the range below equal will be
set. The index into the range is
For "above will be set" and "below equal will be..." I think it would be good to
clarify that you mean the x86 flags register (due to insert cmp instructions).

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4519: typename Traits::Insts::Label *Label;
Is there a more informative name for this? It's used for out of range (and when
there's no Default).

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4527: // Skip over jump table logic if
copmarison not in range and no default
copmarison -> comparison

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4544: Constant *Offset =
Ctx->getConstantSym(0, JumpTable->getName(Func), true);
We usually use

constexpr RelocOffsetT Offset = 0;
constexpr bool SuppressMangling = true;
Ctx->getConstantSym(Offset, ...->getName(Func), SuppressMangling);

so that it's more clear what the parameters are.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4625: // Doing it the clever way
At some point here, could describe what the clever way is (binary search)

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4638: SizeT NumCases = Inst->getNumCases();
Maybe factor out to a function and share the code for now.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4684: SearchSpan(SizeT _Begin, SizeT _Size,
There's still some debate on what to do with shadowing:
https://codereview.chromium.org/1221643012/, and how to name the variables to
get around that.

Otherwise, for now the style is still to just shadow. So change _Begin -> Begin,
etc.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4697: SearchSpan span =
SearchSpanStack.top();
"span" -> "Span"

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4713:
lowerCaseCluster(CaseClusters[span.Begin  ], Comparison,
nit: Hmm haven't seen spacing attempt to line up the [ ], so I would just keep
it tight "[span.Begin], Comparison".

[Jim Stichnoth, 2015-07-15 19:17:43]

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp#newcode218
src/IceCfgNode.cpp:218: // not comtain duplicates.
contain

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp#newcode249
src/IceCfgNode.cpp:249: // Repoint all suitable branch instruction's target and
return.
Sorry for the grammar nit...

Repoint all suitable branch instructions' targets and return.

https://codereview.chromium.org/1234803007/diff/1/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceClFlags.h#newcode246
src/IceClFlags.h:246: bool UseAdvancedSwitchLowering;
Sort this alphabetically (before UseSandboxing).

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode467
src/IceInst.cpp:467: std::sort(OutEdges.begin(), OutEdges.end());
What is the comparison function here?  It needs to be something deterministic
and not address-based so that we can ensure deterministic execution across
different runs and even different platforms.

Ideally the comparison would be on node numbers.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode468
src/IceInst.cpp:468: auto last = std::unique(OutEdges.begin(), OutEdges.end());
capitalize Last per LLVM convention

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode519
src/IceInst.cpp:519: Targets[I] = Default;
For safety, maybe all of these could be initialized to nullptr except for the
entry that actually represents the default target?

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode951
src/IceInst.cpp:951: void InstJumpTable::emit(const Cfg *Func) const {
This should be tied in to the target data lowering.

In fact, any subclass of InstHighLevel should not have an emit() or emitIAS()
method (and we should probably declare those as final in InstHighLevel).

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h#newcode936
src/IceInst.h:936: SizeT Number;
Isn't this a duplicate of Inst::Number?  Can it be removed?

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h#newcode937
src/IceInst.h:937: SizeT NumTargets;
Can this be const?

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:1: //===----- subzero/src/IceSwitchLowering.h - Switch
lowering -----*- C++ -*-==//
.cpp , and also don't need the -*- C++ -*- stuff.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:81: bool CaseCluster::tryAppend(const CaseCluster& x)
{
More descriptive argument name than "x"?

Capitalize variable names per LLVM convention.

Run "make format".

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4504: _sub(T, Ctx->getConstantInt32(Min));
Here and below, there seem to be assumptions that the uint64_t Min/Max values
actually fit into 32 bits.  This is either a bug, or needs to be
clarified/asserted.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4547: legalizeToVar(Offset), nullptr, Index,
2);
Same as Jan's comment above, for nullptr and 2.

Also, the "2" refers to the log width of a code pointer, but might this be 3 for
x86-64?  (at least non-sandboxed)

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4548: Variable *Target =
makeReg(getPointerType());
I think you can just set Target=nullptr and the _mov() routine will handle the
makeReg() boilerplate.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4550: _jmp(Target);
Leave a TODO about sandboxing the indirect jump - i.e., mask the Target to a
bundle boundary.

BTW, you'll also need to mark each CfgNode that is a potential target of an
indirect jump, because in sandbox mode, all those nodes need to be emitted
bundle-aligned.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4684: SearchSpan(SizeT _Begin, SizeT _Size,
On 2015/07/15 18:32:02, jvoung wrote:
> There's still some debate on what to do with shadowing:
> https://codereview.chromium.org/1221643012/, and how to name the variables to
> get around that.
> 
> Otherwise, for now the style is still to just shadow. So change _Begin ->
Begin,
> etc.

Also, I believe C/C++ forbid the underscore-uppercase prefix...

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4705: llvm_unreachable("Invalid SearchSpan
size");
In general, use llvm::report_fatal_error() rather than llvm_unreachable().

[Jim Stichnoth, 2015-07-15 20:05:12]

One higher-level comment.  I wonder if this transformation could be done at a
higher level, to avoid a lot of duplication of target-specific compare/branch
lowering?  Perhaps as a pre-lowering pass that produces various InstIcmp and
InstBr instructions?  One problem I see is the original assumption that InstBr
is a terminator instruction and isn't used to early-branch out of the block.

Another idea is to lower it by creating temporary x86 compare and branch
instructions and then lowering those temporary instructions, similar to how
helper calls are created via makeHelperCall() and calling lowerCall() on the
result.  Except this would require hooking into the BoolFolding mechanism which
may not be that straightforward.

Or, a pre-lowering pass could create new control flow based on the existing form
of InstIcmp and InstBr instructions.  The extra complication here is to make
sure the new CfgNodes are nicely linearized into the existing Cfg.

[ascull, 2015-07-16 19:38:46]

Looking into moving switch expansion to a higher level has identified some
issues:

 - InstBr can only branch to CfgNodes with BB internal branches not possible. If
all the CfgNodes were created here there would be quite a lot of new ones which
would slow down other phases.

 - Some of the switch lowering uses knowledge of 64/32-bit target. From my
analysis of switches I haven't found a 64-bit switch with a case whose value is
greater than 0xFFFFFFFF. This means on 32-bit targets a single test of the high
word to 0 is sufficient, but on 64-bit this is unnecessary. The high level
lowering will not have knowledge of the target.

 - Jump table indexing (base+index<<shift) will have to be done in high level
instructions and give more work for the address optimizer to create the good x86
memory operands.

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp#newcode218
src/IceCfgNode.cpp:218: // not comtain duplicates.
On 2015/07/15 18:32:01, jvoung wrote:
> comtain -> contain

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceCfgNode.cpp#newcode249
src/IceCfgNode.cpp:249: // Repoint all suitable branch instruction's target and
return.
On 2015/07/15 19:17:42, stichnot wrote:
> Sorry for the grammar nit...
> 
> Repoint all suitable branch instructions' targets and return.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceClFlags.h#newcode246
src/IceClFlags.h:246: bool UseAdvancedSwitchLowering;
On 2015/07/15 19:17:42, stichnot wrote:
> Sort this alphabetically (before UseSandboxing).

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode467
src/IceInst.cpp:467: std::sort(OutEdges.begin(), OutEdges.end());
On 2015/07/15 19:17:42, stichnot wrote:
> What is the comparison function here?  It needs to be something deterministic
> and not address-based so that we can ensure deterministic execution across
> different runs and even different platforms.
> 
> Ideally the comparison would be on node numbers.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode468
src/IceInst.cpp:468: auto last = std::unique(OutEdges.begin(), OutEdges.end());
On 2015/07/15 19:17:42, stichnot wrote:
> capitalize Last per LLVM convention

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode519
src/IceInst.cpp:519: Targets[I] = Default;
On 2015/07/15 19:17:42, stichnot wrote:
> For safety, maybe all of these could be initialized to nullptr except for the
> entry that actually represents the default target?

Every element of the jump table can be jumped to so if it doesn't match a case
it should go to the default case (since it can be sparse). What do you mean by
safety in this case?

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode951
src/IceInst.cpp:951: void InstJumpTable::emit(const Cfg *Func) const {
On 2015/07/15 19:17:42, stichnot wrote:
> This should be tied in to the target data lowering.
> 
> In fact, any subclass of InstHighLevel should not have an emit() or emitIAS()
> method (and we should probably declare those as final in InstHighLevel).

I'm going to be doing this for emitIAS which is still to come, is it OK to leave
a TODO for now?

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode969
src/IceInst.cpp:969: void InstJumpTable::emitIAS(const Cfg *Func) const {
On 2015/07/15 18:32:01, jvoung wrote:
> add a newline between functions

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode970
src/IceInst.cpp:970: // TODO(ascull): put jump table in the global context for
emision later
On 2015/07/15 18:32:01, jvoung wrote:
> emision -> emission

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h#newcode936
src/IceInst.h:936: SizeT Number;
On 2015/07/15 19:17:42, stichnot wrote:
> Isn't this a duplicate of Inst::Number?  Can it be removed?

No, this is a label number not an instruction number. This number is used to
generate the symbol name so must not change, the instruction numbers can change
thanks to renumberInstructions() so Inst::Number isn't suitable.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h#newcode937
src/IceInst.h:937: SizeT NumTargets;
On 2015/07/15 19:17:42, stichnot wrote:
> Can this be const?

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:1: //===----- subzero/src/IceSwitchLowering.h - Switch
lowering -----*- C++ -*-==//
On 2015/07/15 18:32:01, jvoung wrote:
> .cpp
> 
> and no need for the -*- C++ -*- in the .cpp

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:49: // Small number of cases is trivial
On 2015/07/15 18:32:01, jvoung wrote:
> By "trivial" you mean that loading from the jump table, doing indirect jumps,
> and for NaCl sandboxing, aligning the CfgNode targets of the jump table, seems
> to be not "profitable" compared to direct jumps (which don't require the
target
> to be aligned)?

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:53: // Test for a single jump table
On 2015/07/15 18:32:01, jvoung wrote:
> I think it would be good to document why a single one instead of a tree of
them.
> 
> For a tree of jump tables, we'd have to traverse to the leaves before anything
> happens. So, we're sort of looking for an *unbalanced* tree, where there is a
> bias toward frequently executed code.  The problem is that we don't have
profile
> information to do this *well*. So, this single jump table is a good starting
> point where you can get to the jump table quickly without figuring out how to
> unbalance the tree.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:81: bool CaseCluster::tryAppend(const CaseCluster& x)
{
On 2015/07/15 19:17:42, stichnot wrote:
> More descriptive argument name than "x"?
> 
> Capitalize variable names per LLVM convention.
> 
> Run "make format".

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.cpp#n...
src/IceSwitchLowering.cpp:83: bool can = this->Label == x.Label && this->High+1
== x.Low;
On 2015/07/15 18:32:01, jvoung wrote:
> Start variable name with capital letter ("Can")

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:44: private:
On 2015/07/15 18:32:01, jvoung wrote:
> nit: I tend to prefer just having all the public: section continuous and the
> private: continuous.
> 
> It should be okay to define the getters in terms of the fields before the
fields
> are actually defined?

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceSwitchLowering.h#new...
src/IceSwitchLowering.h:72: bool tryAppend(const CaseCluster& x);
On 2015/07/15 18:32:01, jvoung wrote:
> I think LLVM style puts the & next to the x? Did clang-format get this?

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Base.h
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:210: /// set, if in the range below equal will be
set. The index into the range is
On 2015/07/15 18:32:01, jvoung wrote:
> For "above will be set" and "below equal will be..." I think it would be good
to
> clarify that you mean the x86 flags register (due to insert cmp instructions).

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4504: _sub(T, Ctx->getConstantInt32(Min));
On 2015/07/15 19:17:43, stichnot wrote:
> Here and below, there seem to be assumptions that the uint64_t Min/Max values
> actually fit into 32 bits.  This is either a bug, or needs to be
> clarified/asserted.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4519: typename Traits::Insts::Label *Label;
On 2015/07/15 18:32:02, jvoung wrote:
> Is there a more informative name for this? It's used for out of range (and
when
> there's no Default).

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4527: // Skip over jump table logic if
copmarison not in range and no default
On 2015/07/15 18:32:02, jvoung wrote:
> copmarison -> comparison

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4544: Constant *Offset =
Ctx->getConstantSym(0, JumpTable->getName(Func), true);
On 2015/07/15 18:32:02, jvoung wrote:
> We usually use
> 
> constexpr RelocOffsetT Offset = 0;
> constexpr bool SuppressMangling = true;
> Ctx->getConstantSym(Offset, ...->getName(Func), SuppressMangling);
> 
> so that it's more clear what the parameters are.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4547: legalizeToVar(Offset), nullptr, Index,
2);
On 2015/07/15 19:17:43, stichnot wrote:
> Same as Jan's comment above, for nullptr and 2.
> 
> Also, the "2" refers to the log width of a code pointer, but might this be 3
for
> x86-64?  (at least non-sandboxed)

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4548: Variable *Target =
makeReg(getPointerType());
On 2015/07/15 19:17:42, stichnot wrote:
> I think you can just set Target=nullptr and the _mov() routine will handle the
> makeReg() boilerplate.

But does does _mov() ensure the width will be the width size as as a pointer
which is required for the memory operand?

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4550: _jmp(Target);
On 2015/07/15 19:17:43, stichnot wrote:
> Leave a TODO about sandboxing the indirect jump - i.e., mask the Target to a
> bundle boundary.
> 
> BTW, you'll also need to mark each CfgNode that is a potential target of an
> indirect jump, because in sandbox mode, all those nodes need to be emitted
> bundle-aligned.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4625: // Doing it the clever way
On 2015/07/15 18:32:02, jvoung wrote:
> At some point here, could describe what the clever way is (binary search)

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4638: SizeT NumCases = Inst->getNumCases();
On 2015/07/15 18:32:02, jvoung wrote:
> Maybe factor out to a function and share the code for now.

I'm leaving this for now while we consider the higher level lowering of
switches.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4684: SearchSpan(SizeT _Begin, SizeT _Size,
On 2015/07/15 18:32:02, jvoung wrote:
> There's still some debate on what to do with shadowing:
> https://codereview.chromium.org/1221643012/, and how to name the variables to
> get around that.
> 
> Otherwise, for now the style is still to just shadow. So change _Begin ->
Begin,
> etc.

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4697: SearchSpan span =
SearchSpanStack.top();
On 2015/07/15 18:32:02, jvoung wrote:
> "span" -> "Span"

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4705: llvm_unreachable("Invalid SearchSpan
size");
On 2015/07/15 19:17:43, stichnot wrote:
> In general, use llvm::report_fatal_error() rather than llvm_unreachable().

Done.

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4713:
lowerCaseCluster(CaseClusters[span.Begin  ], Comparison,
On 2015/07/15 18:32:02, jvoung wrote:
> nit: Hmm haven't seen spacing attempt to line up the [ ], so I would just keep
> it tight "[span.Begin], Comparison".

Done.

[jvoung (off chromium), 2015-07-16 22:27:50]

https://codereview.chromium.org/1234803007/diff/20001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:411:
OutFlags.setUseAdvancedSwitchLowering(::UseAdvancedSwitchLowering);
could swap with UseSandboxing here too for alphabetical order =)

https://codereview.chromium.org/1234803007/diff/20001/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:60: // So, this single jump table is a good starting
could lift some of the comment words from the next line to this line?

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:214: /// contain the result of a comparison with
the Compaison.
Compaison -> Comparison

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:219: /// true if the flags contain the result of
a comparison with the Compaison.
Comparison

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4684: lowerCaseCluster(CaseClusters.front(),
Src0, false, DefaultLabel);
constexpr bool DoneCmp = false
?

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4727: DoneCmp = false;
Would it make sense to set DoneCmp = false between the two, and then just pass
DoneCmp to the next call?

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/adv-switch-opt.ll (right):

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/adv-switch-opt.ll:20: sw.default:                      
                ; preds = %entry
I think you could remove these comments "; preds = %entry", etc.

Less stuff to update if you ever change the case targets, for whatever reason.

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/adv-switch-opt.ll:36: ; CHECK-NEXT: mov
[[BASE:[^,]+]],0x0 f: R_386_32 .rodata.testJumpTable$jumptable
I would just regex match the "f: " part. That's the address of offset within the
mov instruction.

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/adv-switch-opt.ll:106: ; 64-bit switches where the
cases are all 32-bit values should be reduces to a
reduces -> reduced

[ascull, 2015-07-16 22:55:44]

https://codereview.chromium.org/1234803007/diff/20001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceClFlags.cpp#newc...
src/IceClFlags.cpp:411:
OutFlags.setUseAdvancedSwitchLowering(::UseAdvancedSwitchLowering);
On 2015/07/16 22:27:49, jvoung wrote:
> could swap with UseSandboxing here too for alphabetical order =)

Done.

https://codereview.chromium.org/1234803007/diff/20001/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:60: // So, this single jump table is a good starting
On 2015/07/16 22:27:49, jvoung wrote:
> could lift some of the comment words from the next line to this line?

Done.

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:214: /// contain the result of a comparison with
the Compaison.
On 2015/07/16 22:27:49, jvoung wrote:
> Compaison -> Comparison

Done.

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:219: /// true if the flags contain the result of
a comparison with the Compaison.
On 2015/07/16 22:27:49, jvoung wrote:
> Comparison

Done.

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4684: lowerCaseCluster(CaseClusters.front(),
Src0, false, DefaultLabel);
On 2015/07/16 22:27:49, jvoung wrote:
> constexpr bool DoneCmp = false
> ?

Done.

https://codereview.chromium.org/1234803007/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4727: DoneCmp = false;
On 2015/07/16 22:27:49, jvoung wrote:
> Would it make sense to set DoneCmp = false between the two, and then just pass
> DoneCmp to the next call?

Done.

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/adv-switch-opt.ll (right):

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/adv-switch-opt.ll:20: sw.default:                      
                ; preds = %entry
On 2015/07/16 22:27:49, jvoung wrote:
> I think you could remove these comments "; preds = %entry", etc.
> 
> Less stuff to update if you ever change the case targets, for whatever reason.

Done.

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/adv-switch-opt.ll:36: ; CHECK-NEXT: mov
[[BASE:[^,]+]],0x0 f: R_386_32 .rodata.testJumpTable$jumptable
On 2015/07/16 22:27:49, jvoung wrote:
> I would just regex match the "f: " part. That's the address of offset within
the
> mov instruction.

Done.

https://codereview.chromium.org/1234803007/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/adv-switch-opt.ll:106: ; 64-bit switches where the
cases are all 32-bit values should be reduces to a
On 2015/07/16 22:27:49, jvoung wrote:
> reduces -> reduced

Done.

[Jim Stichnoth, 2015-07-17 15:33:48]

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.cpp#newcode951
src/IceInst.cpp:951: void InstJumpTable::emit(const Cfg *Func) const {
On 2015/07/16 19:38:45, ascull wrote:
> On 2015/07/15 19:17:42, stichnot wrote:
> > This should be tied in to the target data lowering.
> > 
> > In fact, any subclass of InstHighLevel should not have an emit() or
emitIAS()
> > method (and we should probably declare those as final in InstHighLevel).
> 
> I'm going to be doing this for emitIAS which is still to come, is it OK to
leave
> a TODO for now?

TODO is fine for now, thanks.

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceInst.h#newcode936
src/IceInst.h:936: SizeT Number;
On 2015/07/16 19:38:45, ascull wrote:
> On 2015/07/15 19:17:42, stichnot wrote:
> > Isn't this a duplicate of Inst::Number?  Can it be removed?
> 
> No, this is a label number not an instruction number. This number is used to
> generate the symbol name so must not change, the instruction numbers can
change
> thanks to renumberInstructions() so Inst::Number isn't suitable.

I see.  Can it be renamed to avoid shadowing/confusion with Inst::Number?

[edit: I see you've already done that, thanks!]

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4548: Variable *Target =
makeReg(getPointerType());
On 2015/07/16 19:38:46, ascull wrote:
> On 2015/07/15 19:17:42, stichnot wrote:
> > I think you can just set Target=nullptr and the _mov() routine will handle
the
> > makeReg() boilerplate.
> 
> But does does _mov() ensure the width will be the width size as as a pointer
> which is required for the memory operand?

Take a look at definition of _mov():
    ...
    if (Dest == nullptr)
      Dest = makeReg(Src0->getType(), RegNum);
    ...
Here Src0 is MemTarget, which was created above with type getPointerType(), so I
think the types are the same.

https://codereview.chromium.org/1234803007/diff/40001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:72: /// used
Can you fix the line-wrap here?  (e.g., M-q in emacs)

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:1: //===--------- subzero/src/IceSwitchLowering.cpp -
Switch lowering ---------==//
For consistency:

//===- subzero/src/ ...

(even though it's not 100% consistent in Subzero and mildly inconsistent in
LLVM)

Same for IceSwitchLowering.h.

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:15: #include "IceSwitchLowering.h"
add a blank line after the file header

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:24: 
remove this blank line?

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.h...
src/IceSwitchLowering.h:29: CaseCluster() = delete;
Also delete default copy ctor and assignment operator (unless they are actually
needed, in which case define them as default).

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.h...
src/IceSwitchLowering.h:70: bool tryAppend(const CaseCluster &x);
change x to New, like the method definition

https://codereview.chromium.org/1234803007/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:81: // TODO(ascull): what size is best for MIPS?
Was it intentional to comment on MIPS in the ARM file, and vice versa?

https://codereview.chromium.org/1234803007/diff/40001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceTypes.def#newcode33
src/IceTypes.def:33: /* enum value,  size, log_2(size), align, # elts, element
type, */     \
How about just removing the "size" column?  Since it can be derived from the
sizeLog2 column.

The only thing is that the size for IceType_void is set to 0, but you could set
its sizeLog2 value to be something special like -1 and check for that in the
typeWidthInBytes computation.

[ascull, 2015-07-17 18:21:29]

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1234803007/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:4548: Variable *Target =
makeReg(getPointerType());
On 2015/07/17 15:33:47, stichnot wrote:
> On 2015/07/16 19:38:46, ascull wrote:
> > On 2015/07/15 19:17:42, stichnot wrote:
> > > I think you can just set Target=nullptr and the _mov() routine will handle
> the
> > > makeReg() boilerplate.
> > 
> > But does does _mov() ensure the width will be the width size as as a pointer
> > which is required for the memory operand?
> 
> Take a look at definition of _mov():
>     ...
>     if (Dest == nullptr)
>       Dest = makeReg(Src0->getType(), RegNum);
>     ...
> Here Src0 is MemTarget, which was created above with type getPointerType(), so
I
> think the types are the same.

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:72: /// used
On 2015/07/17 15:33:47, stichnot wrote:
> Can you fix the line-wrap here?  (e.g., M-q in emacs)

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.cpp
File src/IceSwitchLowering.cpp (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:1: //===--------- subzero/src/IceSwitchLowering.cpp -
Switch lowering ---------==//
On 2015/07/17 15:33:47, stichnot wrote:
> For consistency:
> 
> //===- subzero/src/ ...
> 
> (even though it's not 100% consistent in Subzero and mildly inconsistent in
> LLVM)
> 
> Same for IceSwitchLowering.h.

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:15: #include "IceSwitchLowering.h"
On 2015/07/17 15:33:47, stichnot wrote:
> add a blank line after the file header

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.c...
src/IceSwitchLowering.cpp:24: 
On 2015/07/17 15:33:47, stichnot wrote:
> remove this blank line?

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.h
File src/IceSwitchLowering.h (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.h...
src/IceSwitchLowering.h:29: CaseCluster() = delete;
On 2015/07/17 15:33:47, stichnot wrote:
> Also delete default copy ctor and assignment operator (unless they are
actually
> needed, in which case define them as default).

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceSwitchLowering.h...
src/IceSwitchLowering.h:70: bool tryAppend(const CaseCluster &x);
On 2015/07/17 15:33:47, stichnot wrote:
> change x to New, like the method definition

Done.

https://codereview.chromium.org/1234803007/diff/40001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:81: // TODO(ascull): what size is best for MIPS?
On 2015/07/17 15:33:47, stichnot wrote:
> Was it intentional to comment on MIPS in the ARM file, and vice versa?

No.

https://codereview.chromium.org/1234803007/diff/40001/src/IceTypes.def
File src/IceTypes.def (right):

https://codereview.chromium.org/1234803007/diff/40001/src/IceTypes.def#newcode33
src/IceTypes.def:33: /* enum value,  size, log_2(size), align, # elts, element
type, */     \
On 2015/07/17 15:33:47, stichnot wrote:
> How about just removing the "size" column?  Since it can be derived from the
> sizeLog2 column.
> 
> The only thing is that the size for IceType_void is set to 0, but you could
set
> its sizeLog2 value to be something special like -1 and check for that in the
> typeWidthInBytes computation.

Done.

[jvoung (off chromium), 2015-07-17 20:19:47]

LGTM from my side

[Jim Stichnoth, 2015-07-18 00:13:28]

otherwise lgtm

https://codereview.chromium.org/1234803007/diff/60001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/1234803007/diff/60001/src/IceTypes.cpp#newcod...
src/IceTypes.cpp:137: return (Shift == -1) ? 0 : 1 << Shift;
Probably safer to test for Shift<0 .

[Jim Stichnoth, 2015-07-18 00:14:32]

BTW, were you able to see any performance improvements (or degradations) in
spec2k?

[ascull, 2015-07-20 17:19:02]

There were improvements on most of the spek2 benchmarks. I don't remember all
the details but perl and art benefited well. Art went from around 1.9 to 1.6
(~15% improvement). Any degradation was on the order of 0.01s (insignificant?).

N.B. I didn't run many trials so the numbers might not be accurate but the trend
of being faster should be.

[ascull, 2015-07-20 17:19:20]

Committed patchset #5 (id:80001) manually as
87f80c128a9e2bc95829cd1aac2bbc3c44b44ac1 (presubmit successful).

[Jim Stichnoth, 2015-07-20 18:09:52]

On 2015/07/20 17:19:02, ascull wrote:
> There were improvements on most of the spek2 benchmarks. I don't remember all
> the details but perl and art benefited well. Art went from around 1.9 to 1.6
> (~15% improvement). Any degradation was on the order of 0.01s
(insignificant?).
> 
> N.B. I didn't run many trials so the numbers might not be accurate but the
trend
> of being faster should be.

Here's the methodology I like to use for comparing results.

1. Run on your workstation, and close as many applications as practical,
especially Chrome.

2. Run each spec2k component 5 times, e.g.:
  SPEC_RUN_REPETITIONS=5 ./run_all.sh RunTimedBenchmarks SetupGccX8632Opt train
| tee /tmp/spec.switch.old.txt
  SPEC_RUN_REPETITIONS=5 ./run_all.sh RunTimedBenchmarks SetupGccX8632Opt train
| tee /tmp/spec.switch.new.txt
(rebuilding the spec2k binaries between runs)

3. Grab the data from the "RESULT" output lines and put it into a spreadsheet.

4. Look at both MIN() and AVERAGE() across the 5 runs.

5. Use GEOMEAN() to condense to a single number.

I tried that, and found a geomean improvement of about 0.5%.  Perlbmk did show
3.6% improvement, but art was mostly unchanged.  Gap also showed a 2-3%
improvement.  Eon actually showed a 1.5-3.5% degradation.

[native-client-reviews_googlegroups.com, 2015-07-20 21:13:11]

 I ran the test myself and got very different results (shown below and in
updated spreadsheet). Did you pass `--filetype=asm --sz='--adv-switch'` to
szbuild_spec2k.py? Without that it falls back on the old way of doing
things and your results would just show variation in the runs of the tests.

 min avg  1.08% 2.06%  1.80% 2.73%  18.91% 18.26%  18.35% 17.97%  1.41%
12.04%  19.91% 19.35%  22.45% 22.42%  19.29% 18.45%  18.60% 18.90%  18.02%
16.61%  8.88% 16.76%  1.87% 1.99%  0.85% 1.02%  1.15% 0.30%  1.63% 1.46%
17.98% 17.54%

 10.42% 11.44%

On 20 July 2015 at 11:09, <stichnot@chromium.org> wrote:

> On 2015/07/20 17:19:02, ascull wrote:
>
>> There were improvements on most of the spek2 benchmarks. I don't remember
>> all
>> the details but perl and art benefited well. Art went from around 1.9 to
>> 1.6
>> (~15% improvement). Any degradation was on the order of 0.01s
>>
> (insignificant?).
>
>  N.B. I didn't run many trials so the numbers might not be accurate but the
>>
> trend
>
>> of being faster should be.
>>
>
> Here's the methodology I like to use for comparing results.
>
> 1. Run on your workstation, and close as many applications as practical,
> especially Chrome.
>
> 2. Run each spec2k component 5 times, e.g.:
>   SPEC_RUN_REPETITIONS=5 ./run_all.sh RunTimedBenchmarks SetupGccX8632Opt
> train
> | tee /tmp/spec.switch.old.txt
>   SPEC_RUN_REPETITIONS=5 ./run_all.sh RunTimedBenchmarks SetupGccX8632Opt
> train
> | tee /tmp/spec.switch.new.txt
> (rebuilding the spec2k binaries between runs)
>
> 3. Grab the data from the "RESULT" output lines and put it into a
> spreadsheet.
>
> 4. Look at both MIN() and AVERAGE() across the 5 runs.
>
> 5. Use GEOMEAN() to condense to a single number.
>
> I tried that, and found a geomean improvement of about 0.5%.  Perlbmk did
> show
> 3.6% improvement, but art was mostly unchanged.  Gap also showed a 2-3%
> improvement.  Eon actually showed a 1.5-3.5% degradation.
>
> https://codereview.chromium.org/1234803007/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.