Mark setjmp as "returns twice" and turn off SimpleCoalescing when called.

Mark setjmp as "returns twice" and turn off SimpleCoalescing when called.

BUG=none
R=stichnot@chromium.org

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

[jvoung (off chromium), 2014-09-11 18:49:31]

Patchset #2 (id:20001) has been deleted

[jvoung (off chromium), 2014-09-11 18:53:43]

jvoung@chromium.org changed reviewers:
+ stichnot@chromium.org

[Jim Stichnoth, 2014-09-11 20:12:05]

LGTM, unless you want to implement my suggestion in this CL...

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:37: { Intrinsics::AtomicCmpxchg, true, false },           
                  \
Maybe this CL isn't the right place for this, but it's hard to remember what
attributes and in what order the "true, false" represent, and it's only going to
get worse. :)

Maybe the way to do this is to use symbolic constants like SideEffects_T,
SideEffects_F, ReturnsTwice_T, ReturnsTwice_F.  Define them as values from
separate enums:

enum SideEffects { SideEffects_F=0, SideEffects_T=1 };
enum ReturnTwice { ReturnTwice_F=0, ReturnTwice_T=1 };

Change the definition of IntrinsicInfo:

  struct IntrinsicInfo {
    IntrinsicID ID : 30;
    enum SideEffects HasSideEffects : 1;
    enum ReturnTwice ReturnsTwice : 1;
  };


Then have an initialization macro:
#define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE, ReturnTwice::RT }

Then the initializers look like:

INTRIN(AtomicFence, SideEffects_T, ReturnTwice_F),
INTRIN(AtomicIsLockFree, SideEffects_F, ReturnTwice_F),
...

And if you make a mistake:
INTRIN(AtomicFence, ReturnTwice_F, SideEffects_T),
the compiler will give an error.

Thanks to JF for this approach.  JF also points out that C++ only guarantees
proper packing of bitfields in a struct when the bitfields are the same type, so
they should really be declared as uint32_t and then accessor functions do the
type casting, and you can static_assert that sizeof(IntrinsicInfo)==4.

[jvoung (off chromium), 2014-09-11 21:57:18]

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:37: { Intrinsics::AtomicCmpxchg, true, false },           
                  \
On 2014/09/11 20:12:05, stichnot wrote:
> Maybe this CL isn't the right place for this, but it's hard to remember what
> attributes and in what order the "true, false" represent, and it's only going
to
> get worse. :)
> 
> Maybe the way to do this is to use symbolic constants like SideEffects_T,
> SideEffects_F, ReturnsTwice_T, ReturnsTwice_F.  Define them as values from
> separate enums:
> 
> enum SideEffects { SideEffects_F=0, SideEffects_T=1 };
> enum ReturnTwice { ReturnTwice_F=0, ReturnTwice_T=1 };
> 
> Change the definition of IntrinsicInfo:
> 
>   struct IntrinsicInfo {
>     IntrinsicID ID : 30;
>     enum SideEffects HasSideEffects : 1;
>     enum ReturnTwice ReturnsTwice : 1;
>   };
> 
> 
> Then have an initialization macro:
> #define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE, ReturnTwice::RT
}
> 
> Then the initializers look like:
> 
> INTRIN(AtomicFence, SideEffects_T, ReturnTwice_F),
> INTRIN(AtomicIsLockFree, SideEffects_F, ReturnTwice_F),
> ...
> 
> And if you make a mistake:
> INTRIN(AtomicFence, ReturnTwice_F, SideEffects_T),
> the compiler will give an error.
> 
> Thanks to JF for this approach.  JF also points out that C++ only guarantees
> proper packing of bitfields in a struct when the bitfields are the same type,
so
> they should really be declared as uint32_t and then accessor functions do the
> type casting, and you can static_assert that sizeof(IntrinsicInfo)==4.

Nice! I was worried about losing track of the true/false as well, and the C99 {
.field = <xxx> } isn't allowed in C++. This works nicely.

However, I couldn't do: #define INTRIN(ID, SE, RT) { ... SideEffects::SE ... }

src/IceIntrinsics.cpp:31:46: error: ‘SideEffects’ has not been declared
 #define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE, Intrinsics::RT }
                                              ^
(anyway, that error is silly because I declared the enum within the Intrinsics
class). However, the attempt that could actually refer to SideEffects

#define INTRIN(ID, SE, RT) { ... Intrinsics::SideEffects::SE ... }

also doesn't work:

src/IceIntrinsics.cpp:31:58: error: ‘Ice::Intrinsics::SideEffects’ is not a
class or namespace
 #define INTRIN(ID, SE, RT) { Intrinsics::ID, Intrinsics::SideEffects::SE,
Intrinsics::RT }

So I had to just go with: #define INTRIN(ID, SE, RT) { ... Intrinsics::SE ... }

which would leave only the type-checking of the field's type (so I can't change
it to uint32_t?). Added the STATIC_ASSERT though.

[Jim Stichnoth, 2014-09-11 22:26:26]

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:37: { Intrinsics::AtomicCmpxchg, true, false },           
                  \
On 2014/09/11 21:57:18, jvoung wrote:
> On 2014/09/11 20:12:05, stichnot wrote:
> > Maybe this CL isn't the right place for this, but it's hard to remember what
> > attributes and in what order the "true, false" represent, and it's only
going
> to
> > get worse. :)
> > 
> > Maybe the way to do this is to use symbolic constants like SideEffects_T,
> > SideEffects_F, ReturnsTwice_T, ReturnsTwice_F.  Define them as values from
> > separate enums:
> > 
> > enum SideEffects { SideEffects_F=0, SideEffects_T=1 };
> > enum ReturnTwice { ReturnTwice_F=0, ReturnTwice_T=1 };
> > 
> > Change the definition of IntrinsicInfo:
> > 
> >   struct IntrinsicInfo {
> >     IntrinsicID ID : 30;
> >     enum SideEffects HasSideEffects : 1;
> >     enum ReturnTwice ReturnsTwice : 1;
> >   };
> > 
> > 
> > Then have an initialization macro:
> > #define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE,
ReturnTwice::RT
> }
> > 
> > Then the initializers look like:
> > 
> > INTRIN(AtomicFence, SideEffects_T, ReturnTwice_F),
> > INTRIN(AtomicIsLockFree, SideEffects_F, ReturnTwice_F),
> > ...
> > 
> > And if you make a mistake:
> > INTRIN(AtomicFence, ReturnTwice_F, SideEffects_T),
> > the compiler will give an error.
> > 
> > Thanks to JF for this approach.  JF also points out that C++ only guarantees
> > proper packing of bitfields in a struct when the bitfields are the same
type,
> so
> > they should really be declared as uint32_t and then accessor functions do
the
> > type casting, and you can static_assert that sizeof(IntrinsicInfo)==4.
> 
> Nice! I was worried about losing track of the true/false as well, and the C99
{
> .field = <xxx> } isn't allowed in C++. This works nicely.
> 
> However, I couldn't do: #define INTRIN(ID, SE, RT) { ... SideEffects::SE ... }
> 
> src/IceIntrinsics.cpp:31:46: error: ‘SideEffects’ has not been declared
>  #define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE, Intrinsics::RT
}
>                                               ^
> (anyway, that error is silly because I declared the enum within the Intrinsics
> class). However, the attempt that could actually refer to SideEffects
> 
> #define INTRIN(ID, SE, RT) { ... Intrinsics::SideEffects::SE ... }
> 
> also doesn't work:
> 
> src/IceIntrinsics.cpp:31:58: error: ‘Ice::Intrinsics::SideEffects’ is not a
> class or namespace
>  #define INTRIN(ID, SE, RT) { Intrinsics::ID, Intrinsics::SideEffects::SE,
> Intrinsics::RT }
> 
> So I had to just go with: #define INTRIN(ID, SE, RT) { ... Intrinsics::SE ...
}
> 
> which would leave only the type-checking of the field's type (so I can't
change
> it to uint32_t?). Added the STATIC_ASSERT though.

Oh, right.  I forgot to relay JF's other suggestion.  Change IntrinsicInfo to be
a class, make the (uint32_t) fields private, use a constructor with various enum
type arguments to make the type checking work out, cast them to uint32_t in the
constructor, and add accessor functions that cast from uint32_t to the proper
enum type.

https://codereview.chromium.org/567553003/diff/80001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/567553003/diff/80001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:69: "nacl.atomic.load." NameSuffix                        
             \
line up the backslashes

[jvoung (off chromium), 2014-09-11 22:58:57]

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/567553003/diff/60001/src/IceIntrinsics.cpp#ne...
src/IceIntrinsics.cpp:37: { Intrinsics::AtomicCmpxchg, true, false },           
                  \
On 2014/09/11 22:26:26, stichnot wrote:
> On 2014/09/11 21:57:18, jvoung wrote:
> > On 2014/09/11 20:12:05, stichnot wrote:
> > > Maybe this CL isn't the right place for this, but it's hard to remember
what
> > > attributes and in what order the "true, false" represent, and it's only
> going
> > to
> > > get worse. :)
> > > 
> > > Maybe the way to do this is to use symbolic constants like SideEffects_T,
> > > SideEffects_F, ReturnsTwice_T, ReturnsTwice_F.  Define them as values from
> > > separate enums:
> > > 
> > > enum SideEffects { SideEffects_F=0, SideEffects_T=1 };
> > > enum ReturnTwice { ReturnTwice_F=0, ReturnTwice_T=1 };
> > > 
> > > Change the definition of IntrinsicInfo:
> > > 
> > >   struct IntrinsicInfo {
> > >     IntrinsicID ID : 30;
> > >     enum SideEffects HasSideEffects : 1;
> > >     enum ReturnTwice ReturnsTwice : 1;
> > >   };
> > > 
> > > 
> > > Then have an initialization macro:
> > > #define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE,
> ReturnTwice::RT
> > }
> > > 
> > > Then the initializers look like:
> > > 
> > > INTRIN(AtomicFence, SideEffects_T, ReturnTwice_F),
> > > INTRIN(AtomicIsLockFree, SideEffects_F, ReturnTwice_F),
> > > ...
> > > 
> > > And if you make a mistake:
> > > INTRIN(AtomicFence, ReturnTwice_F, SideEffects_T),
> > > the compiler will give an error.
> > > 
> > > Thanks to JF for this approach.  JF also points out that C++ only
guarantees
> > > proper packing of bitfields in a struct when the bitfields are the same
> type,
> > so
> > > they should really be declared as uint32_t and then accessor functions do
> the
> > > type casting, and you can static_assert that sizeof(IntrinsicInfo)==4.
> > 
> > Nice! I was worried about losing track of the true/false as well, and the
C99
> {
> > .field = <xxx> } isn't allowed in C++. This works nicely.
> > 
> > However, I couldn't do: #define INTRIN(ID, SE, RT) { ... SideEffects::SE ...
}
> > 
> > src/IceIntrinsics.cpp:31:46: error: ‘SideEffects’ has not been declared
> >  #define INTRIN(ID, SE, RT) { Intrinsics::ID, SideEffects::SE,
Intrinsics::RT
> }
> >                                               ^
> > (anyway, that error is silly because I declared the enum within the
Intrinsics
> > class). However, the attempt that could actually refer to SideEffects
> > 
> > #define INTRIN(ID, SE, RT) { ... Intrinsics::SideEffects::SE ... }
> > 
> > also doesn't work:
> > 
> > src/IceIntrinsics.cpp:31:58: error: ‘Ice::Intrinsics::SideEffects’ is not a
> > class or namespace
> >  #define INTRIN(ID, SE, RT) { Intrinsics::ID, Intrinsics::SideEffects::SE,
> > Intrinsics::RT }
> > 
> > So I had to just go with: #define INTRIN(ID, SE, RT) { ... Intrinsics::SE
...
> }
> > 
> > which would leave only the type-checking of the field's type (so I can't
> change
> > it to uint32_t?). Added the STATIC_ASSERT though.
> 
> Oh, right.  I forgot to relay JF's other suggestion.  Change IntrinsicInfo to
be
> a class, make the (uint32_t) fields private, use a constructor with various
enum
> type arguments to make the type checking work out, cast them to uint32_t in
the
> constructor, and add accessor functions that cast from uint32_t to the proper
> enum type.

I thought about using a class + ctor, but doesn't that make this non-POD and add
a static initializer?

It looks like c++11 does allow the Intrinsics::SideEffects::SE, so I could
switch to that when we start compiling with c++11.

[jvoung (off chromium), 2014-09-12 02:18:09]

Committed patchset #5 (id:100001) manually as 44d53e1 (tree was closed).