Concurrency support for PNaCl ABI

include/llvm/IR/Intrinsics.td

Index: include/llvm/IR/Intrinsics.td
diff --git a/include/llvm/IR/Intrinsics.td b/include/llvm/IR/Intrinsics.td
index 3e496204350e8fc932d2070af8be7a8ea7fff48a..2dfcc8e6b6e467763d20e1da79eb0c5116e6da91 100644
--- a/include/llvm/IR/Intrinsics.td
+++ b/include/llvm/IR/Intrinsics.td
@@ -498,6 +498,46 @@ def int_nacl_tp_tdb_offset : Intrinsic<[llvm_i32_ty], [llvm_i32_ty]>,
 // pnaclintrin.h.
 def int_nacl_target_arch : Intrinsic<[llvm_i32_ty], []>,
                             GCCBuiltin<"__builtin_nacl_target_arch">;
+
+// Atomic intrinsics.
+//
+// Volatiles and atomics are encoded through these intrinsics to make
+// them platform-independent, remove some of LLVM's legacy, and isolate
+// PNaCl from future changes to IR. The intrinsics allow user code to
+// use `__sync_*` builtins as well as C11/C++11 atomics.
+//
+// The general signature is:
+// template<typename T>
+// T nacl.atomic.<size>(int32_t operation, T *location, T value,
+//                      T old_value, int32_t memory_order);
+//
+// Where `T` is a 8, 16, 32 or 64-bit integer (`size` bits), and
+// location is naturally aligned to `T`. Valid `operation` and
+// `memory_order` values are in llvm/IR/NaCl.h.
+//
+// Note that not all arguments are meaningful for all operations:

[Mark Seaborn, 2013/06/26 14:33:41]

I'm not keen on having this be overloaded so that it encodes multiple
operations, with different arities, into a single intrinsic.  Having unused
arguments seems untidy.

I think there should be separate intrinsics for atomicrmw, cmpxchg and fence.

[JF, 2013/06/26 15:52:29]

We had this discussion last week, before I started coding anything, and I said
exactly what you are advocating now. You convinced me to do what I've
implemented instead: specialize per-bitwidth, but not per number of arguments.

[Mark Seaborn, 2013/06/26 16:47:01]

I was talking about whether we should have a separate intrinsic for each
"atomicrmw" opcode, i.e.:

  declare i32 @llvm.nacl.atomicrmw.add.i32(i32* %ptr, i32 %val)
  declare i32 @llvm.nacl.atomicrmw.sub.i32(i32* %ptr, i32 %val)
  ...

or whether we have an opcode parameter for "atomicrmw", i.e.

  declare i32 @llvm.nacl.atomicrmw.i32(i32 %opcode, i32* %ptr, i32 %val)
  ; where %opcode represents "add", "sub", etc.

I think the latter is slightly preferable.  If you have a fast-and-dumb code
generator that converts these to function calls, it's a bit easier to have one
function that implements all the opcodes, and it doesn't really constrain
cleverer code generators.

I wasn't talking about whether "atomicrmw" and "cmpxchg" should be merged into
the same intrinsic.

I highly recommend writing design proposals down if you want to get good
feedback!  Either on an issue, or in a doc for longer proposals.  Verbal
discussions don't have the same precision.

[JF, 2013/06/26 22:56:36]

When I confirm that we agree multiple times, and I mention multiple times that
there will be exactly four intrinsics, one per size, I assume we have an
understanding. We write too many things in too many places, and it's occurred
more than once that I write things down only to see you ignore them. I don't
like no-win situations, and this is one.

Again, we had this discussion, I proposed one intrinsic per size (i8, i16, i32,
i64) times one per signature (load, store, rmw, cmpxchg, fence). You said you
preferred less intrinsics, and through further discussion we agreed that having
just 4 would also be cleaner when LLVM evolves.

I'd rather stick with how it is now: LLVM's atomics and volatiles have
pre-C11/C++11 legacy, and I can see them changing at the next major bitcode ABI
change. I think the current implementation is likely to remain stable through
such a change.

[Mark Seaborn, 2013/06/27 01:04:33]

I slightly preferred fewer variants specifically for cmpxchg.  Maybe I took
"just 4" to mean "load atomic", "store atomic", "cmpxchg" and "atomicrmw"?  I
had been hoping to see a design doc or rough outline on
https://code.google.com/p/nativeclient/issues/detail?id=3475 before an
implementation.

Anyway, I think the set of intrinsics should look something like this:

 declare i32 @llvm.nacl.load.atomic.i32(i32 %order, i32* %ptr)
 declare void @llvm.nacl.store.atomic.i32(i32 %order, i32* %ptr, i32 %val)
 declare i32 @llvm.nacl.cmpxchg.i32(i32 %order, i32* %ptr, i32 %oldval, i32
%newval)
 declare i32 @llvm.nacl.atomicrmw.i32(i32 %order, i32* %ptr, i32 %val, i32
%opcode)
 declare void @llvm.nacl.fence(i32 %order)

And then the same again for i8 operations (and later i16 when the validators
support it).

Maybe the names should be have ".p0i32" rather than ".i32" for consistency with
memcpy/memset/etc.

I don't mind whether the intrinsics have a %order argument for the memory
ordering or not.

I don't mind if atomicrmw is split into multiple intrinsics without %opcode,
i.e.
 declare i32 @llvm.nacl.atomicrmw.add.i32(i32 %order, i32* %ptr, i32 %val)
 declare i32 @llvm.nacl.atomicrmw.sub.i32(i32 %order, i32* %ptr, i32 %val)
etc.
-- The idea of having %opcode is only a mild preference.

[JF, 2013/06/27 01:31:39]

No, I definitely said one per size, at multiple occasions.

+//  - result = load {atomic|volatile} T* location, align sizeof(T)
+//  - store {atomic|volatile} T value, T* location memory_order
+//  - result = atomicrmw OP T* location, T value memory_order
+//    Where OP is one of: {add, sub, or, and, xor, xchg}
+//  - result = cmpxchg T* location, T old_value, T value memory_order
+//  - fence memory_order
+def int_nacl_atomic_8 : Intrinsic<[llvm_i8_ty],
+    [llvm_i32_ty, LLVMPointerType<llvm_i8_ty>,
+    llvm_i8_ty, llvm_i8_ty, llvm_i32_ty],
+    [IntrReadWriteArgMem]>;
+def int_nacl_atomic_16 : Intrinsic<[llvm_i16_ty],
+    [llvm_i32_ty, LLVMPointerType<llvm_i16_ty>,
+    llvm_i16_ty, llvm_i16_ty, llvm_i32_ty],
+    [IntrReadWriteArgMem]>;
+def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty],

[Mark Seaborn, 2013/06/26 14:33:41]

You shouldn't need one definition per int size here.  Look at how the
*_with_overflow intrinsics work in this file.  LLVM allows defining polymorphic
intrinsics.

[JF, 2013/06/26 15:52:29]

We kind of do: iAny applies to i1 through i128 as well as v2i1 through v16i64.
We only want 4 of these 34 integer types. I could add a new ValueType, but
that's a bigger local modification than I think we want to support.

[Mark Seaborn, 2013/06/26 16:47:01]

I don't think the definition in Intrinsics.td needs to constrain which int types
the atomic intrinsics can operate on, if that's what you mean.

That's the job of the ABI verifier, which already limits pointer types to i8*,
i16*, i32* and i64*.

[JF, 2013/06/26 22:56:36]

That could be done, we'd then have exactly one intrinsic. Is there a solid
argument for going this way, and forego the typechecks that Intrinsics.td gives
us? As I explain above we had an agreement on 4 intrinsics. Why the change?

[Mark Seaborn, 2013/06/27 01:04:33]

I'm not sure if we're talking at cross purposes here.  This is just an
implementation detail and doesn't affect the PNaCl ABI.  If you have a family of
intrinsics that are parameterised by size, e.g.

  declare void @llvm.foo.i8(i8)
  declare void @llvm.foo.i16(i16)
  declare void @llvm.foo.i32(i32)

then in Intrinsics.td you can either declare this with a single declaration:

  def int_foo : Intrinsic<[], [llvm_anyint_ty]>

or multiple:

  def int_foo_i8 : Intrinsic<[], [llvm_i8_ty]>
  def int_foo_i16 : Intrinsic<[], [llvm_i16_ty]>
  def int_foo_i32 : Intrinsic<[], [llvm_i32_ty]>

I think LLVM always uses the former for its own intrinsics.  We don't
necessarily have to follow this style for PNaCl, but it might make the code more
concise?

[JF, 2013/06/27 01:31:39]

I can implement an overloaded intrinsic, and add the type restriction in
PNaClABIVerifyModule.cpp. I'm merely pointing out that:
 - We had an agreement, this revisit is.
 - Your proposal isn't actually though through.
 - Typechecks will be needed.

Intrinsics.td will do *some* type checking, but not all of it, with overloading,
whereas the current non-overloaded intrinsics get *all* their type checking.
I'll do the change, but I hope you understand my frustration at working through
this again. It's not a question of writing things down either: when I do write
things down there are still misunderstandings with you.

+    [llvm_i32_ty, LLVMPointerType<llvm_i32_ty>,
+    llvm_i32_ty, llvm_i32_ty, llvm_i32_ty],
+    [IntrReadWriteArgMem]>;
+def int_nacl_atomic_64 : Intrinsic<[llvm_i64_ty],
+    [llvm_i32_ty, LLVMPointerType<llvm_i64_ty>,
+    llvm_i64_ty, llvm_i64_ty, llvm_i32_ty],
+    [IntrReadWriteArgMem]>;
 // @LOCALMOD-END
 
 //===----------------------------------------------------------------------===//