Subzero: Reduce the amount of #ifdef'd code.

Subzero: Reduce the amount of #ifdef'd code.

Try to make most #ifdef'd code be compiled under all configurations,
to catch code rot earlier.  When #ifdef code is required, try to use
it only to guard trivial code like "return;".

BUG= none
R=jpp@chromium.org

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

[Jim Stichnoth, 2015-06-20 01:06:45]

Cleanup

[Jim Stichnoth, 2015-06-20 03:36:51]

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

[Karl, 2015-06-22 15:41:07]

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:82: #ifdef NDEBUG
Why not create a boolean (inline) function in IceDefs.h, and then use

if (IsNdebug()) return;

Or, create a constexpr and use that as the test?

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:99: #ifdef NDEBUG
same here.

[John, 2015-06-22 17:13:46]

Hi, I am John Paul and I have a problem:

(Hi John Paul)

I don't like c macros. I prefer c++ constexpr instead.

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:82: #ifdef NDEBUG
On 2015/06/22 15:41:06, Karl wrote:
> Why not create a boolean (inline) function in IceDefs.h, and then use
> 
> if (IsNdebug()) return;
> 
> Or, create a constexpr and use that as the test?

I agree with Karl on this one. Perhaps IceDefs could have something like:

constexpr bool nDebug() {
#if NDEBUG
  return true;
#else
  return false;
#endif  // NDEBUG
}

constexpr bool allowDump() {
if ALLOW_DUMP
  return true;
#else
  return false;
#endif  // ALLOW_DUMP
}

so that we could avoid these macros everywhere.

Additionally, it would be nice to have something like


class BuildTraits {
private:
  template <class Debug, class Release, bool>
  class TypePickerImpl {
  public:
    using Type = Release;
  };

  template <class Debug, class Release>
  class TypePickerImpl<Debug, Release, false> {
  public:
    using Type = Debug;
  };

public:
  static constexpr bool ReleaseBuild = nDebug();
  static constexpr bool AllowDump = allowDump();

  // ReleaseOrDebug is bad... perhaps SecondIfDebug?
  template <class Release, class Debug>
  using ReleaseOrDebug = typename
      TypePickerImpl<Debug, Release, ReleaseBuild>::Type;
};

Which means the ifs would become

if (BuildTraits::AllowDump) { /* do work */}

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:99: #ifdef NDEBUG
On 2015/06/22 15:41:06, Karl wrote:
> same here.

This makes me nervous that, with NDEBUG, some compiler might complain about
unreachable code:

return true;
return HasEnsuredCapacity;

https://codereview.chromium.org/1197863003/diff/40001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/1197863003/diff/40001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:98: if (false) {
Can this be a constexpr bool somewhere, perhaps modified by a compile-time
constant (e.g., -DINEFFICIENT_LIVE_RANGE_CHECK)?

[Jim Stichnoth, 2015-06-23 21:39:00]

Use constexpr inline functions instead of macros

[Jim Stichnoth, 2015-06-23 21:42:08]

I changed almost all uses of macros to uses of constexpr inline functions,
defined in IceDefs.h.  This includes the "if (ALLOW_DUMP)" pattern.

One macro that I can't really change is PNACL_BROWSER_TRANSLATOR.

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h
File src/IceAssembler.h (right):

https://codereview.chromium.org/1197863003/diff/40001/src/IceAssembler.h#newc...
src/IceAssembler.h:82: #ifdef NDEBUG
On 2015/06/22 15:41:06, Karl wrote:
> Why not create a boolean (inline) function in IceDefs.h, and then use
> 
> if (IsNdebug()) return;
> 
> Or, create a constexpr and use that as the test?

Did both, in IceDefs.h:
  constexpr bool buildAllowsAsserts() { ... }
  constexpr bool buildAllowsExtraValidation() { ... }
Also:
  constexpr bool buildAllowsDump() { ... }

https://codereview.chromium.org/1197863003/diff/40001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/1197863003/diff/40001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:98: if (false) {
On 2015/06/22 17:13:46, John wrote:
> Can this be a constexpr bool somewhere, perhaps modified by a compile-time
> constant (e.g., -DINEFFICIENT_LIVE_RANGE_CHECK)?

Done.

[John, 2015-06-23 22:28:30]

lgtm

Just make sure the #ifdef stuff is right. lgtm otherwise.

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssembler.cpp
File src/IceAssembler.cpp (right):

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssembler.cpp#ne...
src/IceAssembler.cpp:48: void
AssemblerBuffer::EnsureCapacity::validate(AssemblerBuffer *buffer) {
optional: s/buffer/Buffer/

?

If you want to avoid the name clash, then perhaps

s/buffer/Buf/

?

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssembler.cpp#ne...
src/IceAssembler.cpp:68: (void)delta;
optional: s/delta/Delta/?

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssemblerX8632.cpp
File src/IceAssemblerX8632.cpp (right):

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssemblerX8632.c...
src/IceAssemblerX8632.cpp:36: for (const Label *Label : CfgNodeLabels) {
optional: perhaps a good case for auto?

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssemblerX8632.h
File src/IceAssemblerX8632.h (right):

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssemblerX8632.h...
src/IceAssemblerX8632.h:256: for (int i = 0; i < kMaxUnresolvedBranches; i++) {
optional: s/i/I/

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

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode48
src/IceDefs.h:48: constexpr bool buildAllowsAsserts() {
optional: As we spoke during our walk back, it seems these functions are all
part of

namespace BuildAllows {
...
}

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode49
src/IceDefs.h:49: #ifdef NDEBUG
This is a bit surprising to me. I always though

-DNDEBUG=0

would turn asserts on... oh, the wonders of C. :)

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode57
src/IceDefs.h:57: #ifdef ALLOW_EXTRA_VALIDATION
probably just

#if ALLOW_EXTRA_VALIDATION

?

Otherwise,

-DALLOW_EXTRA_VALIDATION=0

will cause extra validation to be allowed.

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode65
src/IceDefs.h:65: #if defined(ALLOW_DUMP) && ALLOW_DUMP
this can be simply

#if ALLOW_DUMP

(see  https://gcc.gnu.org/onlinedocs/gcc-3.0.2/cpp_4.html)

[Jim Stichnoth, 2015-06-24 21:27:27]

PTAL

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssembler.cpp
File src/IceAssembler.cpp (right):

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssembler.cpp#ne...
src/IceAssembler.cpp:48: void
AssemblerBuffer::EnsureCapacity::validate(AssemblerBuffer *buffer) {
On 2015/06/23 22:28:30, John wrote:
> optional: s/buffer/Buffer/
> 
> ?
> 
> If you want to avoid the name clash, then perhaps
> 
> s/buffer/Buf/
> 
> ?

I think these naming inconsistencies come from trying to import Dart code while
minimizing changes, for easier merging in the future.  I know we've diverged a
bit, but I'm still hoping to keep that minimized (despite this CL's non-cosmetic
changes).

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssembler.cpp#ne...
src/IceAssembler.cpp:68: (void)delta;
On 2015/06/23 22:28:30, John wrote:
> optional: s/delta/Delta/?

(same response as above)

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssemblerX8632.h
File src/IceAssemblerX8632.h (right):

https://codereview.chromium.org/1197863003/diff/60001/src/IceAssemblerX8632.h...
src/IceAssemblerX8632.h:256: for (int i = 0; i < kMaxUnresolvedBranches; i++) {
On 2015/06/23 22:28:30, John wrote:
> optional: s/i/I/

Even LLVM violates its own written rules in cases like this.  (git grep -w i --
*.cpp)

The exception pattern seems to be that single-character integral loop index
variables are often lowercase, while e.g. uppercase I is more commonly used for
complex iterators, or Instruction objection.

It's really hard to bring myself to use "int I"...

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

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode48
src/IceDefs.h:48: constexpr bool buildAllowsAsserts() {
On 2015/06/23 22:28:30, John wrote:
> optional: As we spoke during our walk back, it seems these functions are all
> part of
> 
> namespace BuildAllows {
> ...
> }
> 

Done.  I went with BuildDefs::asserts(), and similar for all the other defines
where that was practical.

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode49
src/IceDefs.h:49: #ifdef NDEBUG
On 2015/06/23 22:28:30, John wrote:
> This is a bit surprising to me. I always though
> 
> -DNDEBUG=0
> 
> would turn asserts on... oh, the wonders of C. :)

Yep, NDEBUG is either defined or not, and its value matters not...

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode57
src/IceDefs.h:57: #ifdef ALLOW_EXTRA_VALIDATION
On 2015/06/23 22:28:30, John wrote:
> probably just
> 
> #if ALLOW_EXTRA_VALIDATION
> 
> ?
> 
> Otherwise,
> 
> -DALLOW_EXTRA_VALIDATION=0
> 
> will cause extra validation to be allowed.

Done.

https://codereview.chromium.org/1197863003/diff/60001/src/IceDefs.h#newcode65
src/IceDefs.h:65: #if defined(ALLOW_DUMP) && ALLOW_DUMP
On 2015/06/23 22:28:30, John wrote:
> this can be simply
> 
> #if ALLOW_DUMP
> 
> (see  https://gcc.gnu.org/onlinedocs/gcc-3.0.2/cpp_4.html)

Done.

[John, 2015-06-24 22:44:33]

lgtm

[Jim Stichnoth, 2015-06-24 22:52:31]

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