Enable Local CSE by default

Enable Local CSE by default

Reduce the default number of iterations to 1
Put the optional code behind the -lcse-no-ssa flag, which is disabled by
default. This brings down the overhead of enabling this to about 2%.

BUG=
R=stichnot@chromium.org

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

[manasijm, 2016-07-27 22:02:55]

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

[John, 2016-07-28 14:21:51]

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode575
src/IceCfg.cpp:575: bool NoSSA = getFlags().getLocalCSENoSSA();
const bool

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode589
src/IceCfg.cpp:589: // TODO : Stats on IterCount -> performance
TODO(manasijm):

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode593
src/IceCfg.cpp:593: if (NoSSA) {
document what's going on here. why is this needed for non-SSA IR?

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode603
src/IceCfg.cpp:603: for (auto DepInst : DepIter->second) {
auto *

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

https://codereview.chromium.org/2185193002/diff/20001/src/IceClFlags.def#newc...
src/IceClFlags.def:143: X(LocalCSE, bool, dev_opt_flag, "lcse",                 
                    \
nit: instead of two bool flags (lcse, lcse-no-ssa), I would suggest using a
single, enum flag (e.g., -target). I would name the flag cse, and the values,
{no, yes, yes-no-assume-cse)

[manasijm, 2016-07-28 15:24:44]

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode593
src/IceCfg.cpp:593: if (NoSSA) {
On 2016/07/28 14:21:50, John wrote:
> document what's going on here. why is this needed for non-SSA IR?

This is needed if this pass is moved to a point later in the pipeline.
If Variables have a single definition in the block, CSE can work just on the
basis of an equality compare on instructions (sans Dest). When variables can be
updated (hence, non-SSA) the result of a previous instruction which used that
variable can not be reused.
(Will put this as a comment here.)

[manasijm, 2016-07-28 19:43:34]

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode575
src/IceCfg.cpp:575: bool NoSSA = getFlags().getLocalCSENoSSA();
On 2016/07/28 14:21:50, John wrote:
> const bool

Done.

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode589
src/IceCfg.cpp:589: // TODO : Stats on IterCount -> performance
On 2016/07/28 14:21:50, John wrote:
> TODO(manasijm):

Done.

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode593
src/IceCfg.cpp:593: if (NoSSA) {
On 2016/07/28 15:24:44, manasijm wrote:
> On 2016/07/28 14:21:50, John wrote:
> > document what's going on here. why is this needed for non-SSA IR?
> 
> This is needed if this pass is moved to a point later in the pipeline.
> If Variables have a single definition in the block, CSE can work just on the
> basis of an equality compare on instructions (sans Dest). When variables can
be
> updated (hence, non-SSA) the result of a previous instruction which used that
> variable can not be reused.
> (Will put this as a comment here.)

Done.

https://codereview.chromium.org/2185193002/diff/20001/src/IceCfg.cpp#newcode603
src/IceCfg.cpp:603: for (auto DepInst : DepIter->second) {
On 2016/07/28 14:21:50, John wrote:
> auto *

Done.

[Jim Stichnoth, 2016-07-29 14:49:02]

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.cpp#newcode588
src/IceCfg.cpp:588: // Not necessary for SSA
Can you describe this with respect to the arg name/condition?  Also, might note
that it should consume minimal memory when unused.

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.cpp#newcode592
src/IceCfg.cpp:592: while (IterCount--) {
When I look at this loop, I get nervous about off-by-one errors, pre-increment
versus post-increment, signed versus unsigned, etc.  How about a more standard
looping structure?

const uint32_t IterCount = ...;
for (uint32_t i = 0; i < IterCount; ++i) { ... }

This also suggests changing the flag in IceClFlags.def from int to uint32_t.

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.h#newcode204
src/IceCfg.h:204: void localCSE(const bool NoSSA);
I would remove the "const" here.

Also, I think it's easier to understand when parameters like this convey the
positive sense rather than the negative sense.  Otherwise I might end up calling
it like this:

  static constexpr bool NoNoSSA = false;
  localCse(NoNoSSA);

E.g. call it "bool UseSSA" or "AssumeSSA" and invert the logic everywhere.

https://codereview.chromium.org/2185193002/diff/60001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceClFlags.def#newc...
src/IceClFlags.def:144: cl::desc("Local Common Subexpression elimination"),     
                  \
I would use all lowercase apart from "Local".

https://codereview.chromium.org/2185193002/diff/60001/src/IceClFlags.def#newc...
src/IceClFlags.def:148: clEnumValN(Ice::LCSEOptions::EnabledSSA, "enabled",
"assume-ssa"),     \
unindent

https://codereview.chromium.org/2185193002/diff/60001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceDefs.h#newcode311
src/IceDefs.h:311: enum class LCSEOptions {
Bikeshed: For consistency with these other enums, I would use "enum" instead of
"enum class", and then pick a "unique" short prefix for the enum values.  E.g.:

enum LcseOptions {
  LCSE_Disabled,
  LCSE_EnabledSSA,
  LCSE_EnabledNoSSA
};

I agree though that "enum class" is a cleaner way to deal with the global
namespace pollution.  This is probably a legacy of pre-C++11 code.

[manasijm, 2016-08-01 17:39:24]

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.cpp#newcode588
src/IceCfg.cpp:588: // Not necessary for SSA
On 2016/07/29 14:49:02, stichnot wrote:
> Can you describe this with respect to the arg name/condition?  Also, might
note
> that it should consume minimal memory when unused.

Done.

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.cpp#newcode592
src/IceCfg.cpp:592: while (IterCount--) {
On 2016/07/29 14:49:02, stichnot wrote:
> When I look at this loop, I get nervous about off-by-one errors, pre-increment
> versus post-increment, signed versus unsigned, etc.  How about a more standard
> looping structure?
> 
> const uint32_t IterCount = ...;
> for (uint32_t i = 0; i < IterCount; ++i) { ... }
> 
> This also suggests changing the flag in IceClFlags.def from int to uint32_t.

Done.

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceCfg.h#newcode204
src/IceCfg.h:204: void localCSE(const bool NoSSA);
On 2016/07/29 14:49:02, stichnot wrote:
> I would remove the "const" here.
> 
> Also, I think it's easier to understand when parameters like this convey the
> positive sense rather than the negative sense.  Otherwise I might end up
calling
> it like this:
> 
>   static constexpr bool NoNoSSA = false;
>   localCse(NoNoSSA);
> 
> E.g. call it "bool UseSSA" or "AssumeSSA" and invert the logic everywhere.

Done.

https://codereview.chromium.org/2185193002/diff/60001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceClFlags.def#newc...
src/IceClFlags.def:144: cl::desc("Local Common Subexpression elimination"),     
                  \
On 2016/07/29 14:49:02, stichnot wrote:
> I would use all lowercase apart from "Local".

Done.

https://codereview.chromium.org/2185193002/diff/60001/src/IceClFlags.def#newc...
src/IceClFlags.def:148: clEnumValN(Ice::LCSEOptions::EnabledSSA, "enabled",
"assume-ssa"),     \
On 2016/07/29 14:49:02, stichnot wrote:
> unindent

Done.

https://codereview.chromium.org/2185193002/diff/60001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/2185193002/diff/60001/src/IceDefs.h#newcode311
src/IceDefs.h:311: enum class LCSEOptions {
On 2016/07/29 14:49:02, stichnot wrote:
> Bikeshed: For consistency with these other enums, I would use "enum" instead
of
> "enum class", and then pick a "unique" short prefix for the enum values. 
E.g.:
> 
> enum LcseOptions {
>   LCSE_Disabled,
>   LCSE_EnabledSSA,
>   LCSE_EnabledNoSSA
> };
> 
> I agree though that "enum class" is a cleaner way to deal with the global
> namespace pollution.  This is probably a legacy of pre-C++11 code.

Done.

[Jim Stichnoth, 2016-08-01 20:25:36]

otherwise lgtm

https://codereview.chromium.org/2185193002/diff/80001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/2185193002/diff/80001/src/IceCfg.cpp#newcode598
src/IceCfg.cpp:598: for (SizeT i = 0; i < IterCount; ++i) {
I would declare this as uint32_t instead of SizeT, for consistency with how
getLocalCseMaxIterations() is defined.

https://codereview.chromium.org/2185193002/diff/80001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/2185193002/diff/80001/src/IceClFlags.def#newc...
src/IceClFlags.def:149: clEnumValN(Ice::LCSE_EnabledNoSSA,"no-ssa" ,
"no-assume-ssa"),           \
Fix the spaces here?  No space before the comma, one space after.

[manasijm, 2016-08-01 22:40:45]

Description was changed from

==========
Enable Local CSE by default

Reduce the default number of iterations to 1
Put the optional code behind the -lcse-no-ssa flag, which is disabled by
default. This brings down the overhead of enabling this to about 2%.

BUG=
==========

to

==========
Enable Local CSE by default

Reduce the default number of iterations to 1
Put the optional code behind the -lcse-no-ssa flag, which is disabled by
default. This brings down the overhead of enabling this to about 2%.

BUG=
R=stichnot@chromium.org

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

[manasijm, 2016-08-01 22:40:46]

Committed patchset #5 (id:80001) manually as
53c8fbdf9af4673bb3e674ed5c9d09e899edabf8 (presubmit successful).