[PATCH] D123143: SelectionDAG: Swap operands of atomic_store

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
----------------
nemanjai wrote:
> 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.
It's only silent if you have no test coverage. This also wouldn't help the tablegen patterns at all


================
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;
----------------
nemanjai wrote:
> What do we gain by reordering the defs of the load and store in this patch?
Keeps the two store and load cases together. Right now the atomic cases are together, but the atomics are closer to aliases


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

https://reviews.llvm.org/D123143