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

src/IceIntrinsics.cpp

@@ -24,39 +24,49 @@ namespace Ice {
 
 namespace {
 
+void __attribute__((unused)) xIntrinsicInfoSizeCheck() {
+  STATIC_ASSERT(sizeof(Intrinsics::IntrinsicInfo) == 4);
+}
+
+#define INTRIN(ID, SE, RT) { Intrinsics::ID, Intrinsics::SE, Intrinsics::RT }
+
+// 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 },                                     \
+      INTRIN(AtomicCmpxchg, SideEffects_T, ReturnsTwice_F),                    \
       { Overload, IceType_i32, Overload, Overload, IceType_i32, IceType_i32 }, \
-      6                                                                        \
-    }                                                                          \
-    , "nacl.atomic.cmpxchg." NameSuffix                                        \
+      6 },                                                                     \
+    "nacl.atomic.cmpxchg." NameSuffix                                          \
   }
     AtomicCmpxchgInit(IceType_i8, "i8"),
     AtomicCmpxchgInit(IceType_i16, "i16"),
     AtomicCmpxchgInit(IceType_i32, "i32"),
     AtomicCmpxchgInit(IceType_i64, "i64"),
 #undef AtomicCmpxchgInit
-    { { { Intrinsics::AtomicFence, true }, { IceType_void, IceType_i32 }, 2 },
+
+    { { INTRIN(AtomicFence, SideEffects_T, ReturnsTwice_F),
+        { IceType_void, IceType_i32 }, 2 },
       "nacl.atomic.fence" },
-    { { { Intrinsics::AtomicFenceAll, true }, { IceType_void }, 1 },
+    { { INTRIN(AtomicFenceAll, SideEffects_T, ReturnsTwice_F),
+        { IceType_void }, 1 },
       "nacl.atomic.fence.all" },
-    { { { Intrinsics::AtomicIsLockFree, false },
+    { { INTRIN(AtomicIsLockFree, SideEffects_F, ReturnsTwice_F),
         { IceType_i1, IceType_i32, IceType_i32 }, 3 },
       "nacl.atomic.is.lock.free" },
 
 #define AtomicLoadInit(Overload, NameSuffix)                                   \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::AtomicLoad, true }                                         \
-      , { Overload, IceType_i32, IceType_i32 }, 3                              \
-    }                                                                          \
-    , "nacl.atomic.load." NameSuffix                                           \
+      INTRIN(AtomicLoad, SideEffects_T, ReturnsTwice_F),                       \
+      { Overload, IceType_i32, IceType_i32 }, 3 },                             \
+    "nacl.atomic.load." NameSuffix                                             \
   }
     AtomicLoadInit(IceType_i8, "i8"),
     AtomicLoadInit(IceType_i16, "i16"),
@@ -67,7 +77,7 @@ const struct IceIntrinsicsEntry_ {
 #define AtomicRMWInit(Overload, NameSuffix)                                    \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::AtomicRMW, true }                                          \
+      INTRIN(AtomicRMW, SideEffects_T, ReturnsTwice_F)                         \
       , { Overload, IceType_i32, IceType_i32, Overload, IceType_i32 }, 5       \
     }                                                                          \
     , "nacl.atomic.rmw." NameSuffix                                            \
@@ -81,7 +91,7 @@ const struct IceIntrinsicsEntry_ {
 #define AtomicStoreInit(Overload, NameSuffix)                                  \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::AtomicStore, true }                                        \
+      INTRIN(AtomicStore, SideEffects_T, ReturnsTwice_F)                       \
       , { IceType_void, Overload, IceType_i32, IceType_i32 }, 4                \
     }                                                                          \
     , "nacl.atomic.store." NameSuffix                                          \
@@ -95,7 +105,7 @@ const struct IceIntrinsicsEntry_ {
 #define BswapInit(Overload, NameSuffix)                                        \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::Bswap, false }                                             \
+      INTRIN(Bswap, SideEffects_F, ReturnsTwice_F)                             \
       , { Overload, Overload }, 2                                              \
     }                                                                          \
     , "bswap." NameSuffix                                                      \
@@ -108,7 +118,7 @@ const struct IceIntrinsicsEntry_ {
 #define CtlzInit(Overload, NameSuffix)                                         \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::Ctlz, false }                                              \
+      INTRIN(Ctlz, SideEffects_F, ReturnsTwice_F)                              \
       , { Overload, Overload, IceType_i1 }, 3                                  \
     }                                                                          \
     , "ctlz." NameSuffix                                                       \
@@ -120,7 +130,7 @@ const struct IceIntrinsicsEntry_ {
 #define CtpopInit(Overload, NameSuffix)                                        \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::Ctpop, false }                                             \
+      INTRIN(Ctpop, SideEffects_F, ReturnsTwice_F)                             \
       , { Overload, Overload }, 2                                              \
     }                                                                          \
     , "ctpop." NameSuffix                                                      \
@@ -132,7 +142,7 @@ const struct IceIntrinsicsEntry_ {
 #define CttzInit(Overload, NameSuffix)                                         \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::Cttz, false }                                              \
+      INTRIN(Cttz, SideEffects_F, ReturnsTwice_F)                              \
       , { Overload, Overload, IceType_i1 }, 3                                  \
     }                                                                          \
     , "cttz." NameSuffix                                                       \
@@ -140,45 +150,57 @@ const struct IceIntrinsicsEntry_ {
     CttzInit(IceType_i32, "i32"),
     CttzInit(IceType_i64, "i64"),
 #undef CttzInit
-    { { { Intrinsics::Longjmp, true },
+
+    { { INTRIN(Longjmp, SideEffects_T, ReturnsTwice_F),
         { IceType_void, IceType_i32, IceType_i32 }, 3 },
       "nacl.longjmp" },
-    { { { Intrinsics::Memcpy, true }, { IceType_void, IceType_i32, IceType_i32,
-                                        IceType_i32,  IceType_i32, IceType_i1 },
+    { { INTRIN(Memcpy, SideEffects_T, ReturnsTwice_F),
+        { 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 },
+    { { INTRIN(Memmove, SideEffects_T, ReturnsTwice_F),
+        { 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 },
+    { { INTRIN(Memset, SideEffects_T, ReturnsTwice_F),
+        { IceType_void, IceType_i32, IceType_i8, IceType_i32, IceType_i32,
+          IceType_i1 },
         6 },
       "memset.p0i8.i32" },
-    { { { Intrinsics::NaClReadTP, false }, { IceType_i32 }, 1 },
+    { { INTRIN(NaClReadTP, SideEffects_F, ReturnsTwice_F),
+        { IceType_i32 }, 1 },
       "nacl.read.tp" },
-    { { { Intrinsics::Setjmp, true }, { IceType_i32, IceType_i32 }, 2 },
+    { { INTRIN(Setjmp, SideEffects_T, ReturnsTwice_T),
+        { IceType_i32, IceType_i32 }, 2 },
       "nacl.setjmp" },
 
 #define SqrtInit(Overload, NameSuffix)                                         \
   {                                                                            \
     {                                                                          \
-      { Intrinsics::Sqrt, false }                                              \
-      , { Overload, Overload }, 2                                              \
-    }                                                                          \
-    , "sqrt." NameSuffix                                                       \
+      INTRIN(Sqrt, SideEffects_F, ReturnsTwice_F),                             \
+      { Overload, Overload }, 2 },                                             \
+      "sqrt." NameSuffix                                                       \
   }
     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 },
+
+    { { INTRIN(Stacksave, SideEffects_T, ReturnsTwice_F),
+        { IceType_i32 }, 1 },
+      "stacksave" },
+    { { INTRIN(Stackrestore, SideEffects_T, ReturnsTwice_F),
+        { IceType_void, IceType_i32 }, 2 },
       "stackrestore" },
-    { { { Intrinsics::Trap, true }, { IceType_void }, 1 }, "trap" }
-  };
+    { { INTRIN(Trap, SideEffects_T, ReturnsTwice_F),
+        { IceType_void }, 1 },
+      "trap" }
+};
 const size_t IceIntrinsicsTableSize = llvm::array_lengthof(IceIntrinsicsTable);
 
+#undef INTRIN
+
 } // end of anonymous namespace
 
 Intrinsics::Intrinsics() {

src/IceIntrinsics.h

@@ -88,15 +88,24 @@ public:
 
   static bool VerifyMemoryOrder(uint64_t Order);
 
+  enum SideEffects {
+    SideEffects_F=0,
+    SideEffects_T=1
+  };
+
+  enum ReturnsTwice {
+    ReturnsTwice_F=0,
+    ReturnsTwice_T=1
+  };
+
   // Basic attributes related to each intrinsic, that are relevant to
-  // code generation. We will want to have more attributes (e.g., Setjmp
-  // returns twice and which affects stack coloring) once the lowering
-  // cares about such attributes. Perhaps the attributes representation
-  // can be shared with general function calls, though most functions
-  // will be opaque.
+  // code generation. Perhaps the attributes representation can be shared
+  // with general function calls, but PNaCl currently strips all
+  // attributes from functions.
   struct IntrinsicInfo {
-    IntrinsicID ID : 31;
-    bool HasSideEffects : 1;
+    enum IntrinsicID ID : 30;
+    enum SideEffects HasSideEffects : 1;
+    enum ReturnsTwice ReturnsTwice : 1;
   };
 
   // The complete set of information about an intrinsic.

src/IceTargetLowering.cpp

@@ -171,9 +171,13 @@ void TargetLowering::lower() {
   case Inst::InsertElement:
     lowerInsertElement(llvm::dyn_cast<InstInsertElement>(Inst));
     break;
-  case Inst::IntrinsicCall:
-    lowerIntrinsicCall(llvm::dyn_cast<InstIntrinsicCall>(Inst));
+  case Inst::IntrinsicCall: {
+    InstIntrinsicCall *Call = llvm::dyn_cast<InstIntrinsicCall>(Inst);
+    if (Call->getIntrinsicInfo().ReturnsTwice)
+      setCallsReturnsTwice(true);
+    lowerIntrinsicCall(Call);
     break;
+  }
   case Inst::Load:
     lowerLoad(llvm::dyn_cast<InstLoad>(Inst));
     break;

src/IceTargetLowering.h

@@ -141,6 +141,12 @@ public:
   virtual llvm::ArrayRef<uint8_t> getNonExecBundlePadding() const = 0;
   bool hasComputedFrame() const { return HasComputedFrame; }
   bool shouldDoNopInsertion() const;
+  // Returns true if this function calls a function that has the
+  // "returns twice" attribute.
+  bool callsReturnsTwice() const { return CallsReturnsTwice; }
+  void setCallsReturnsTwice(bool RetTwice) {
+    CallsReturnsTwice = RetTwice;
+  }
   int32_t getStackAdjustment() const { return StackAdjustment; }
   void updateStackAdjustment(int32_t Offset) { StackAdjustment += Offset; }
   void resetStackAdjustment() { StackAdjustment = 0; }
@@ -176,7 +182,7 @@ public:
 protected:
   TargetLowering(Cfg *Func)
       : Func(Func), Ctx(Func->getContext()), HasComputedFrame(false),
-        StackAdjustment(0) {}
+        CallsReturnsTwice(false), StackAdjustment(0) {}
   virtual void lowerAlloca(const InstAlloca *Inst) = 0;
   virtual void lowerArithmetic(const InstArithmetic *Inst) = 0;
   virtual void lowerAssign(const InstAssign *Inst) = 0;
@@ -210,6 +216,7 @@ protected:
   Cfg *Func;
   GlobalContext *Ctx;
   bool HasComputedFrame;
+  bool CallsReturnsTwice;
   // StackAdjustment keeps track of the current stack offset from its
   // natural location, as arguments are pushed for a function call.
   int32_t StackAdjustment;

src/IceTargetLoweringX8632.cpp

@@ -647,9 +647,13 @@ void TargetX8632::addProlog(CfgNode *Node) {
   // frames.  If SimpleCoalescing is true, then each "global" variable
   // without a register gets its own slot, but "local" variable slots
   // are reused across basic blocks.  E.g., if A and B are local to
-  // block 1 and C is local to block 2, then C may share a slot with A
-  // or B.
-  const bool SimpleCoalescing = true;
+  // block 1 and C is local to block 2, then C may share a slot with A or B.
+  //
+  // We cannot coalesce stack slots if this function calls a "returns twice"
+  // function. In that case, basic blocks may be revisited, and variables
+  // local to those basic blocks are actually live until after the
+  // called function returns a second time.
+  const bool SimpleCoalescing = !callsReturnsTwice();
   size_t InArgsSizeBytes = 0;
   size_t PreservedRegsSizeBytes = 0;
   SpillAreaSizeBytes = 0;

tests_lit/llvm2ice_tests/returns_twice_no_coalesce.ll

+; This file checks that SimpleCoalescing of local stack slots is not done
+; when calling a function with the "returns twice" attribute.
+
+; RUN: %llvm2ice -Om1 --verbose none %s \
+; RUN:   | llvm-mc -triple=i686-none-nacl -x86-asm-syntax=intel -filetype=obj \
+; RUN:   | llvm-objdump -d --symbolize -x86-asm-syntax=intel - | FileCheck %s
+; RUN: %llvm2ice --verbose none %s | FileCheck --check-prefix=ERRORS %s
+
+; Setjmp is a function with the "returns twice" attribute.
+declare i32 @llvm.nacl.setjmp(i8*)
+
+declare i32 @other(i32)
+declare void @user(i32)
+
+define i32 @call_returns_twice(i32 %iptr_jmpbuf, i32 %x) {
+entry:
+  %local = add i32 %x, 12345
+  %jmpbuf = inttoptr i32 %iptr_jmpbuf to i8*
+  %y = call i32 @llvm.nacl.setjmp(i8* %jmpbuf)
+  call void @user(i32 %local)
+  %cmp = icmp eq i32 %y, 0
+  br i1 %cmp, label %Zero, label %NonZero
+Zero:
+  %other_local = add i32 %x, 54321
+  call void @user(i32 %other_local)
+  ret i32 %other_local
+NonZero:
+  ret i32 1
+}
+
+; CHECK-LABEL: call_returns_twice
+; CHECK: add [[REG1:.*]], 12345
+; CHECK: mov dword ptr [esp + [[OFF:.*]]], [[REG1]]
+; CHECK: add [[REG2:.*]], 54321
+; There should not be sharing of the stack slot.
+; CHECK-NOT: mov dword ptr [esp + [[OFF]]], [[REG2]]
+
+define i32 @no_call_returns_twice(i32 %iptr_jmpbuf, i32 %x) {
+entry:
+  %local = add i32 %x, 12345
+  %y = call i32 @other(i32 %x)
+  call void @user(i32 %local)
+  %cmp = icmp eq i32 %y, 0
+  br i1 %cmp, label %Zero, label %NonZero
+Zero:
+  %other_local = add i32 %x, 54321
+  call void @user(i32 %other_local)
+  ret i32 %other_local
+NonZero:
+  ret i32 1
+}
+
+; CHECK-LABEL: no_call_returns_twice
+; CHECK: add [[REG1:.*]], 12345
+; CHECK: mov dword ptr [esp + [[OFF:.*]]], [[REG1]]
+; CHECK: add [[REG2:.*]], 54321
+; Now there should be sharing of the stack slot (OFF is the same).
+; CHECK: mov dword ptr [esp + [[OFF]]], [[REG2]]
+
+; ERRORS-NOT: ICE translation error