[PATCH] D18775: NFC: make AtomicOrdering an enum class

[JF Bastien via llvm-commits]

jfb added a comment.

In http://reviews.llvm.org/D18775#393220, @jyknight wrote:

> I'm a bit wondering what was the point of making it an enum class? It looks like what you were going for is to remove < and > comparisons with the AtomicOrdering values, and to always use more explicit accessors. But, enum class doesn't actually prevent that, right?


Correct, it doesn't prevent it. I want to get this basic change in, and then figure out what the best approach is. I'm not a fan of creating a class for this purpose, seems too heave a solution.

One thing I can do is:

  void operator<(AtomicOrdering, AtomicOrdering) {}
  void operator>(AtomicOrdering, AtomicOrdering) {}
  void operator<=(AtomicOrdering, AtomicOrdering) {}
  void operator>=(AtomicOrdering, AtomicOrdering) {}

That'll require fixing the uses of comparison that are OK right now.

WDYT? I can do it in this patch if you want.


================
Comment at: include/llvm/IR/Instructions.h:300
@@ -270,2 +299,3 @@
   void setOrdering(AtomicOrdering Ordering) {
     setInstructionSubclassData((getSubclassDataFromInstruction() & ~(7 << 7)) |
+                               ((unsigned)Ordering << 7));
----------------
jyknight wrote:
> This comment is not actually related to your patch:
> 
> I notice this code here (and in other cases below) is depending on the AtomicOrdering values fitting in *3* bits, while the code in AtomicSDNode is checking that it fits in *4* bits. That seems an unfortunate discrepency.
Right, there are a few such things around the codebase :-)

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:521
@@ -520,3 +520,3 @@
       // FIXME: This is overly conservative.
-      if (LI->isAtomic() && LI->getOrdering() > Unordered) {
+      if (LI->isAtomic() && isStrongerThanMonotonic(LI->getOrdering())) {
         if (!QueryInst || isNonSimpleLoadOrStore(QueryInst) ||
----------------
jyknight wrote:
> Is this an unintentional change? It used to be enforcing ordering requirements on Monotonic or stronger, not only stronger-than Monotonic.
Oof yes, good catch! Fixed.


http://reviews.llvm.org/D18775