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

[James Y Knight via llvm-commits]

jyknight added a comment.

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?


================
Comment at: include/llvm/IR/Instructions.h:76
@@ +75,3 @@
+
+/// Returns true if the ordering is stronger than Monotonic (i.e. consume,
+/// acquire, acq_rel, or seq_cst).
----------------
the "i.e."s here seem pretty obvious from the code

================
Comment at: include/llvm/IR/Instructions.h:300
@@ -270,2 +299,3 @@
   void setOrdering(AtomicOrdering Ordering) {
     setInstructionSubclassData((getSubclassDataFromInstruction() & ~(7 << 7)) |
+                               ((unsigned)Ordering << 7));
----------------
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.

================
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) ||
----------------
Is this an unintentional change? It used to be enforcing ordering requirements on Monotonic or stronger, not only stronger-than Monotonic.


http://reviews.llvm.org/D18775