[PATCH] D57601: [WIP] Seperate volatility and atomicity/ordering in SelectionDAG

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
Herald added subscribers: jfb, bollu, mcrosier.

At the moment, we mark every atomic memory access as being also volatile.  This is unnecessarily conservative and prohibits many legal transforms (DCE, folding, etc..).

This patch is build on top of a series of other patches which adjust SelectionDAG code to (for the moment) be as conservative for an atomic access as for a volatile access.  There are a couple already out for review, with more to follow:

- D57593 <https://reviews.llvm.org/D57593> Fix a bug in the definition of isUnordered on MachineMemOperand
- D57596 <https://reviews.llvm.org/D57596> [CodeGen] Be conservative about atomic accesses as for volatile

The intention once all of these land is to apply this patch as an NFC.  (i.e. there should be no change in behaviour associated with this patch)  Once it's in and baked for a bit, we'll start working through removing unnecessary bailouts one by one.  We applied this same strategy to the middle end a few years ago, with good success.

Disturbingly, this patch in it's current state - which is known to be incorrect due to not having the supporting patches references above - does not currently trigger any test failures in a make check run.  That clearly needs fixed before this can be landed.


https://reviews.llvm.org/D57601

Files:
  lib/CodeGen/SelectionDAG/SelectionDAG.cpp
  lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp


Index: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4278,6 +4278,7 @@
   AtomicOrdering Order = I.getOrdering();
   SyncScope::ID SSID = I.getSyncScopeID();
 
+  // FIXME: The chaining of atomic loads is unneccessarily conservative
   SDValue InChain = getRoot();
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -6397,10 +6397,7 @@
 
   MachineFunction &MF = getMachineFunction();
 
-  // FIXME: Volatile isn't really correct; we should keep track of atomic
-  // orderings in the memoperand.
-  auto Flags = MachineMemOperand::MOVolatile | MachineMemOperand::MOLoad |
-               MachineMemOperand::MOStore;
+  auto Flags = MachineMemOperand::MOLoad | MachineMemOperand::MOStore;
   MachineMemOperand *MMO =
     MF.getMachineMemOperand(PtrInfo, Flags, MemVT.getStoreSize(), Alignment,
                             AAMDNodes(), nullptr, SSID, SuccessOrdering,
@@ -6432,11 +6429,7 @@
   MachineFunction &MF = getMachineFunction();
   // An atomic store does not load. An atomic load does not store.
   // (An atomicrmw obviously both loads and stores.)
-  // For now, atomics are considered to be volatile always, and they are
-  // chained as such.
-  // FIXME: Volatile isn't really correct; we should keep track of atomic
-  // orderings in the memoperand.
-  auto Flags = MachineMemOperand::MOVolatile;
+  auto Flags = MachineMemOperand::MONone;
   if (Opcode != ISD::ATOMIC_STORE)
     Flags |= MachineMemOperand::MOLoad;
   if (Opcode != ISD::ATOMIC_LOAD)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57601.184781.patch
Type: text/x-patch
Size: 1874 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190201/485156b9/attachment.bin>