Subzero: Support non sequentially consistent memory orderings for atomic ops.

src/IceIntrinsics.cpp

Index: src/IceIntrinsics.cpp
diff --git a/src/IceIntrinsics.cpp b/src/IceIntrinsics.cpp
index 26a81dea7eaa10b5836e1aac95d29011c9396db9..7c5df5d3afd1cab3c3b81544718fd76723fc9e89 100644
--- a/src/IceIntrinsics.cpp
+++ b/src/IceIntrinsics.cpp
@@ -233,7 +233,55 @@ const Intrinsics::FullIntrinsicInfo *Intrinsics::find(const IceString &Name,
   return &it->second;
 }
 
-bool Intrinsics::VerifyMemoryOrder(uint64_t Order) {
+bool Intrinsics::VerifyMemoryOrder(IntrinsicID ID, uint64_t Order,
+                                   uint64_t OrderOther) {

[JF, 2015/03/17 16:00:07]

Rename to isMemoryOrderValid?

[Jim Stichnoth, 2015/03/17 16:44:56]

Done.

+  if (Order == Intrinsics::MemoryOrderInvalid)
+    return false;
+  if (Order == Intrinsics::MemoryOrderNum)
+    return false;
+  switch (ID) {
+  default:
+    llvm_unreachable("VerifyMemoryOrder: Unknown IntrinsicID");
+    return false;
+    break;

[JF, 2015/03/17 16:00:07]

unreachable, return false *and* break! Porque no les tres?

Can it be reached if the input file is bad? Or does the decoder prevent this?

[Jim Stichnoth, 2015/03/17 16:44:56]

20 goto 10

But yeah, break was left over from a previous iteration...

Leaving the "return false" because (as I understand it) llvm_unreachable()
doesn't guarantee an abort.
Given that nacl-atomic-errors.ll explicitly tests these errors, I would say the
parser is not validating this...

[JF, 2015/03/17 16:58:53]

It's truly unreachable if you whitelist above :-)
It's between you and Karl, but shouldn't the reader be the one deciding what's
invalid?

[Jim Stichnoth, 2015/03/17 18:42:04]

Actually the ID can be for any intrinsic, not just atomic intrinsics.
I agree that ultimately the reader should be the gatekeeper...

[JF, 2015/03/17 21:18:14]

Bug?

[JF, 2015/03/18 06:26:30]

No bug for this? :(

[Jim Stichnoth, 2015/03/18 13:50:15]

Done.

+  case AtomicFence:
+  case AtomicFenceAll:
+  case AtomicRMW:
+    return true;

[JF, 2015/03/17 16:00:07]

You have to reject relaxed and consume, though you should do this at the very
top (PNaCl restriction, not C++ restriction).

[Jim Stichnoth, 2015/03/17 16:44:56]

Oops, done.

BTW, I should have brought this up in issue 4029, but is it really OK that we
are taking C++11 rules and applying them to all languages/compilers that target
PNaCl?

[JF, 2015/03/17 16:58:53]

I think so: they're generally usable as a base to build other languages on top.
It would be nice to have relaxed too, I'm being pretty conservative with that.

+  case AtomicCmpxchg:
+    switch (OrderOther) {
+    case Intrinsics::MemoryOrderInvalid:
+    case Intrinsics::MemoryOrderNum:
+    case Intrinsics::MemoryOrderRelease:
+    case Intrinsics::MemoryOrderAcquireRelease:

[JF, 2015/03/17 16:00:07]

Additional PNaCl restriction: reject relaxed and consume.

[Jim Stichnoth, 2015/03/17 16:44:56]

Done.

+      return false;
+    default:
+      if (OrderOther > Order)
+        return false;
+      if (Order == Intrinsics::MemoryOrderRelease &&
+          OrderOther == Intrinsics::MemoryOrderRelaxed)

[JF, 2015/03/17 16:00:07]

This should be:
  Success == Release && Failure != Relaxed

[Jim Stichnoth, 2015/03/17 16:44:56]

Done.

+        return false;
+      return true;
+    }
+    break;
+  case AtomicLoad:
+    if (Order == Intrinsics::MemoryOrderRelease)
+      return false;
+    if (Order == Intrinsics::MemoryOrderAcquireRelease)
+      return false;
+    return true;
+    break;
+  case AtomicStore:
+    if (Order == Intrinsics::MemoryOrderConsume)
+      return false;
+    if (Order == Intrinsics::MemoryOrderAcquire)
+      return false;
+    if (Order == Intrinsics::MemoryOrderAcquireRelease)
+      return false;
+    return true;
+    break;
+  }
+  return false;

[JF, 2015/03/17 16:00:08]

Is this reachable?

[Jim Stichnoth, 2015/03/17 16:44:56]

Done.

   // There is only one memory ordering for atomics allowed right now.
   return Order == Intrinsics::MemoryOrderSequentiallyConsistent;

[JF, 2015/03/17 16:00:07]

This definitely isn't reachable!

[Jim Stichnoth, 2015/03/17 16:44:56]

Oops, I forgot to remove the old code!

 }