[PATCH] D123143: SelectionDAG: Swap operands of atomic_store

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:1434
 
-  const SDValue &getBasePtr() const { return getOperand(1); }
-  const SDValue &getVal() const { return getOperand(2); }
+  const SDValue &getBasePtr() const {
+    return getOpcode() == ISD::ATOMIC_STORE ? getOperand(2) : getOperand(1);
----------------
I think this is quite problematic for downstream, out of tree code. If I'm not mistaken, the operands of the node simply get reversed, silently causing failures.

Perhaps we should implement new overloads of these and `getOperand()` with a new parameter to indicate that the caller is aware of the change and make the original ones hit an unreachable (this would of course be limited to `ATOMIC_STORE`). Changing all in-tree callers allows this to continue to work whereas any downstream code that is unaware of the change will simply break.


================
Comment at: llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td:183
 def : GINodeEquiv<G_STORE, st> { let CheckMMOIsNonAtomic = true; }
-
-def : GINodeEquiv<G_LOAD, atomic_load> {
+def : GINodeEquiv<G_STORE, atomic_store> {
   let CheckMMOIsNonAtomic = false;
----------------
What do we gain by reordering the defs of the load and store in this patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123143/new/

https://reviews.llvm.org/D123143