[PATCH] D24577: [RFC] Move synchronization scope and atomic orderings from SDNode to MachineMemOperand

[Konstantin Zhuravlyov via llvm-commits]

kzhuravl added inline comments.


================
Comment at: include/llvm/CodeGen/MachineMemOperand.h:131
+    /// Reserved/unused.
+    unsigned Reserved : 23;
+  };
----------------
jlebar wrote:
> I don't think we need this field.
Removed.


================
Comment at: include/llvm/CodeGen/MachineMemOperand.h:151
+    assert(getFailureOrdering() == FailureOrdering && "Value truncated");
+  }
+
----------------
jlebar wrote:
> Can we do this inside the constructor?
Moved inside the constructor.


================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:1198
+    return MMO->getSuccessOrdering();
   }
 
----------------
jlebar wrote:
> Should we make the MMO and AtomicSDNode interfaces match?  Right now we have getOrdering and getSuccessOrdering on MMO, but only getOrdering on the SDNode.
> 
> I think I'd be OK having two functions that both return the success ordering if we assert inside getFailureOrdering / getSuccessOrdering that we have a cmpxchg instruction.  Otherwise maybe having only the one function makes sense.
Added asserts inside getFailureOrdering/getSuccessOrdering.


https://reviews.llvm.org/D24577