src/IceIntrinsics.cpp - Issue 567553003: Mark setjmp as "returns twice" and turn off SimpleCoalescing when called.

Unified Diff: src/IceIntrinsics.cpp

Issue 567553003: Mark setjmp as "returns twice" and turn off SimpleCoalescing when called. (Closed) Base URL: https://chromium.googlesource.com/native_client/pnacl-subzero.git@master

Patch Set: balance brackets Created 6 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: src/IceIntrinsics.cpp

diff --git a/src/IceIntrinsics.cpp b/src/IceIntrinsics.cpp

index 5b6ceac1a341f158f33e1bd1a56a6162d602c67a..e0f6d4c26d8a58ad1fca3546bcf19baacece117c 100644

--- a/src/IceIntrinsics.cpp

+++ b/src/IceIntrinsics.cpp

@@ -24,14 +24,17 @@ namespace Ice {

namespace {

+// Build list of intrinsics with their attributes and expected prototypes.

+// List is sorted alphabetically.

const struct IceIntrinsicsEntry_ {

Intrinsics::FullIntrinsicInfo Info;

const char *IntrinsicName;

} IceIntrinsicsTable[] = {

#define AtomicCmpxchgInit(Overload, NameSuffix) \

{ \

- { Intrinsics::AtomicCmpxchg, true }, \

+ { Intrinsics::AtomicCmpxchg, true, false }, \

Jim Stichnoth 2014/09/11 20:12:05 Maybe this CL isn't the right place for this, but

jvoung (off chromium) 2014/09/11 21:57:18 Nice! I was worried about losing track of the true

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 Oh, right. I forgot to relay JF's other suggestio

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.

jvoung (off chromium) 2014/09/11 22:58:57 I thought about using a class + ctor, but doesn't

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.

{ Overload, IceType_i32, Overload, Overload, IceType_i32, IceType_i32 }, \

6 \

} \

@@ -42,18 +45,20 @@ const struct IceIntrinsicsEntry_ {

AtomicCmpxchgInit(IceType_i32, "i32"),

AtomicCmpxchgInit(IceType_i64, "i64"),

#undef AtomicCmpxchgInit

- { { { Intrinsics::AtomicFence, true }, { IceType_void, IceType_i32 }, 2 },

+ { { { Intrinsics::AtomicFence, true, false }, { IceType_void, IceType_i32 },

+ 2},

"nacl.atomic.fence" },

- { { { Intrinsics::AtomicFenceAll, true }, { IceType_void }, 1 },

+ { { { Intrinsics::AtomicFenceAll, true, false }, { IceType_void }, 1 },

"nacl.atomic.fence.all" },

- { { { Intrinsics::AtomicIsLockFree, false },

+ { { { Intrinsics::AtomicIsLockFree, false, false },

{ IceType_i1, IceType_i32, IceType_i32 }, 3 },

"nacl.atomic.is.lock.free" },

#define AtomicLoadInit(Overload, NameSuffix) \

{ \

- { Intrinsics::AtomicLoad, true } \

+ { Intrinsics::AtomicLoad, true, false } \

, { Overload, IceType_i32, IceType_i32 }, 3 \

} \

, "nacl.atomic.load." NameSuffix \

@@ -67,7 +72,7 @@ const struct IceIntrinsicsEntry_ {

#define AtomicRMWInit(Overload, NameSuffix) \

{ \

- { Intrinsics::AtomicRMW, true } \

+ { Intrinsics::AtomicRMW, true, false } \

, { Overload, IceType_i32, IceType_i32, Overload, IceType_i32 }, 5 \

} \

, "nacl.atomic.rmw." NameSuffix \

@@ -81,7 +86,7 @@ const struct IceIntrinsicsEntry_ {

#define AtomicStoreInit(Overload, NameSuffix) \

{ \

- { Intrinsics::AtomicStore, true } \

+ { Intrinsics::AtomicStore, true, false } \

, { IceType_void, Overload, IceType_i32, IceType_i32 }, 4 \

} \

, "nacl.atomic.store." NameSuffix \

@@ -95,7 +100,7 @@ const struct IceIntrinsicsEntry_ {

#define BswapInit(Overload, NameSuffix) \

{ \

- { Intrinsics::Bswap, false } \

+ { Intrinsics::Bswap, false, false } \

, { Overload, Overload }, 2 \

} \

, "bswap." NameSuffix \

@@ -108,7 +113,7 @@ const struct IceIntrinsicsEntry_ {

#define CtlzInit(Overload, NameSuffix) \

{ \

- { Intrinsics::Ctlz, false } \

+ { Intrinsics::Ctlz, false, false } \

, { Overload, Overload, IceType_i1 }, 3 \

} \

, "ctlz." NameSuffix \

@@ -120,7 +125,7 @@ const struct IceIntrinsicsEntry_ {

#define CtpopInit(Overload, NameSuffix) \

{ \

- { Intrinsics::Ctpop, false } \

+ { Intrinsics::Ctpop, false, false } \

, { Overload, Overload }, 2 \

} \

, "ctpop." NameSuffix \

@@ -132,7 +137,7 @@ const struct IceIntrinsicsEntry_ {

#define CttzInit(Overload, NameSuffix) \

{ \

- { Intrinsics::Cttz, false } \

+ { Intrinsics::Cttz, false, false } \

, { Overload, Overload, IceType_i1 }, 3 \

} \

, "cttz." NameSuffix \

@@ -140,31 +145,34 @@ const struct IceIntrinsicsEntry_ {

CttzInit(IceType_i32, "i32"),

CttzInit(IceType_i64, "i64"),

#undef CttzInit

- { { { Intrinsics::Longjmp, true },

+ { { { Intrinsics::Longjmp, true, false },

{ IceType_void, IceType_i32, IceType_i32 }, 3 },

"nacl.longjmp" },

- { { { Intrinsics::Memcpy, true }, { IceType_void, IceType_i32, IceType_i32,

- IceType_i32, IceType_i32, IceType_i1 },

+ { { { Intrinsics::Memcpy, true, false },

+ { IceType_void, IceType_i32, IceType_i32, IceType_i32, IceType_i32,

+ IceType_i1},

6 },

"memcpy.p0i8.p0i8.i32" },

- { { { Intrinsics::Memmove, true },

- { IceType_void, IceType_i32, IceType_i32,

- IceType_i32, IceType_i32, IceType_i1 },

+ { { { Intrinsics::Memmove, true, false },

+ { IceType_void, IceType_i32, IceType_i32, IceType_i32, IceType_i32,

+ IceType_i1 },

6 },

"memmove.p0i8.p0i8.i32" },

- { { { Intrinsics::Memset, true }, { IceType_void, IceType_i32, IceType_i8,

- IceType_i32, IceType_i32, IceType_i1 },

+ { { { Intrinsics::Memset, true, false },

+ { IceType_void, IceType_i32, IceType_i8, IceType_i32, IceType_i32,

+ IceType_i1 },

6 },

"memset.p0i8.i32" },

- { { { Intrinsics::NaClReadTP, false }, { IceType_i32 }, 1 },

+ { { { Intrinsics::NaClReadTP, false, false }, { IceType_i32 }, 1 },

"nacl.read.tp" },

- { { { Intrinsics::Setjmp, true }, { IceType_i32, IceType_i32 }, 2 },

+ { { { Intrinsics::Setjmp, true, true }, { IceType_i32, IceType_i32 }, 2 },

"nacl.setjmp" },

#define SqrtInit(Overload, NameSuffix) \

{ \

- { Intrinsics::Sqrt, false } \

+ { Intrinsics::Sqrt, false, false } \

, { Overload, Overload }, 2 \

} \

, "sqrt." NameSuffix \

@@ -172,11 +180,15 @@ const struct IceIntrinsicsEntry_ {

SqrtInit(IceType_f32, "f32"),

SqrtInit(IceType_f64, "f64"),

#undef SqrtInit

- { { { Intrinsics::Stacksave, true }, { IceType_i32 }, 1 }, "stacksave" },

- { { { Intrinsics::Stackrestore, true }, { IceType_void, IceType_i32 }, 2 },

+ { { { Intrinsics::Stacksave, true, false }, { IceType_i32 }, 1 },

+ "stacksave" },

+ { { { Intrinsics::Stackrestore, true, false },

+ { IceType_void, IceType_i32 },

+ 2 },

"stackrestore" },

- { { { Intrinsics::Trap, true }, { IceType_void }, 1 }, "trap" }

- };

+ { { { Intrinsics::Trap, true, false }, { IceType_void }, 1 }, "trap" }

+};

const size_t IceIntrinsicsTableSize = llvm::array_lengthof(IceIntrinsicsTable);

} // end of anonymous namespace

« no previous file with comments | « src/IceIntrinsics.h ('k') | src/IceTargetLowering.h » ('j') | no next file with comments »