Seperate volatility and atomicity/ordering in SelectionDAG

[reames]

Summary

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 removes MOVolatile from the MachineMemOperands of atomic, but not volatile, instructions. This should be strictly NFC after a series of previous patches which have gone in to ensure backend code is conservative about handling of isAtomic MMOs. 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.

To make sure this patch itself is NFC, it is build on top of a series of other patches which adjust code to (for the moment) be as conservative for an atomic access as for a volatile access and build up a test corpus (mostly in test/CodeGen/X86/atomics-unordered.ll)..

Previously landed

• D57593 Fix a bug in the definition of isUnordered on MachineMemOperand
• D57596 [CodeGen] Be conservative about atomic accesses as for volatile
• D57802 Be conservative about unordered accesses for the moment
• rL353959: [Tests] First batch of cornercase tests for unordered atomics.
• rL353966: [Tests] RMW folding tests w/unordered atomic operations.
• rL353972: [Tests] More unordered atomic lowering tests.
• rL353989: [SelectionDAG] Inline a single use helper function, and remove last non-MMO interface
• rL354740: [Hexagon, SystemZ] Be super conservative about atomics
• rL354800: [Lanai] Be super conservative about atomics
• rL354845: [ARM] Be super conservative about atomics

Attention Out of Tree Backend Owners: This patch may break you. If it does, you can use the TLI getMMOFlags hook to restore the MOVolatile to any instruction you need to. (See llvm-dev thread titled "PSA: Changes to how atomics are handled in backends" started Feb 27, 2019.)

Work planned for once this is in, and has baked for a bit:

• working through all of the TODOs in terms of code quality in the added tests
• D57803 [X86][GlobalISEL] Support lowering aligned unordered atomics
• ImplicitNullCheck for unordered loads
• load folding for unordered loads
• store folding for unordered stores
• load/store unfolding
• review of the other conservative bails outs inserted in pre-work for this patch

[reames]

Missed a case, and got some failing tests this time. Yeah for test coverage!

[reames]

No longer WIP, ready for real review.

[reames]

Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.

[reames]

ping

[asb]

Adding @jyknight.

It seems there's a clear plan on the path forwards - is it worth writing it up as a short RFC on the mailing list, particular given the potential for subtle issues for out-of-tree backends?

[reames]

In D57601#1401075, @asb wrote:

It seems there's a clear plan on the path forwards - is it worth writing it up as a short RFC on the mailing list, particular given the potential for subtle issues for out-of-tree backends?

I was planning on sending a note after it lands. Happy to do so now if folks things it's warranted.

[jyknight]

In D57601#1397317, @reames wrote:

Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.

I think adding the isAtomic check to the appropriate locations that are checking isVolatile makes the most sense, since it's easier to see and undo in the future, if that's desired.

[reames]

In D57601#1404320, @jyknight wrote:

In D57601#1397317, @reames wrote:

Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.

I think adding the isAtomic check to the appropriate locations that are checking isVolatile makes the most sense, since it's easier to see and undo in the future, if that's desired.

D58490 is posted for review.

[reames]

ping

[jlebar]

I'm not totally comfortable with this code, but this change looks reasonable to me.

[reames]

When preparing to land the approved patch, I discovered one last issue. Essentially, the previously committed fixes for SystemZ and XCore are insufficient. Both backends do something odd which is they "lower" an AtomicSDNode into the corresponding *non* atomic LoadSDNode, and then let that be selected in turn. This results in a LoadSDNode w/an atomic (but non-volatile) memory operand. None of the DAG combines expect that. Amusingly, the test which fails actually has correct output (i.e. we DSE an atomic store), but I really want this patch to be non-functional.

The fix is simple: opt both backends out of the change for the moment. I could do a more fine grained fix, but I'd like to land this as is, specifically to demonstrate that the opt-out works for any downstream backends which are effected.

p.s. Sorry for not noticing this earlier. I must have not built these backends w/all of the various patches applied.

[reames]

Helps if I upload the current diff...

[jlebar]

Still LGTM.

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitAtomicCmpXchg(const AtomicCmpXchgInst &I) {
 
   SDValue InChain = getRoot();
 
   MVT MemVT = getValue(I.getCompareOperand()).getSimpleValueType();
   SDVTList VTs = DAG.getVTList(MemVT, MVT::i1, MVT::Other);
 
   auto Alignment = DAG.getEVTAlignment(MemVT);
 
-  // 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;
+  if (I.isVolatile())
+    Flags |= MachineMemOperand::MOVolatile;
+  Flags |= DAG.getTargetLoweringInfo().getMMOFlags(I);
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineMemOperand *MMO =
     MF.getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()),
                             Flags, MemVT.getStoreSize(), Alignment,
                             AAMDNodes(), nullptr, SSID, SuccessOrdering,
                             FailureOrdering);
 
void SelectionDAGBuilder::visitAtomicRMW(const AtomicRMWInst &I) {
   AtomicOrdering Ordering = I.getOrdering();
   SyncScope::ID SSID = I.getSyncScopeID();
 
   SDValue InChain = getRoot();
 
   auto MemVT = getValue(I.getValOperand()).getSimpleValueType();
   auto Alignment = DAG.getEVTAlignment(MemVT);
 
-  // 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 |
-    MachineMemOperand::MOLoad |  MachineMemOperand::MOStore;
+  auto Flags = MachineMemOperand::MOLoad |  MachineMemOperand::MOStore;
+  if (I.isVolatile())
+    Flags |= MachineMemOperand::MOVolatile;
+  Flags |= DAG.getTargetLoweringInfo().getMMOFlags(I);
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineMemOperand *MMO =
     MF.getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()), Flags,
                             MemVT.getStoreSize(), Alignment, AAMDNodes(),
                             nullptr, SSID, Ordering);
 
   SDValue L =
void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   EVT VT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 
   if (!TLI.supportsUnalignedAtomics() &&
       I.getAlignment() < VT.getStoreSize())
     report_fatal_error("Cannot generate unaligned atomic load");
 
+  auto Flags = MachineMemOperand::MOLoad;
+  if (I.isVolatile())
+    Flags |= MachineMemOperand::MOVolatile;
+  Flags |= TLI.getMMOFlags(I);
+
   MachineMemOperand *MMO =
       DAG.getMachineFunction().
       getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()),
-                           MachineMemOperand::MOVolatile |
-                           MachineMemOperand::MOLoad,
-                           VT.getStoreSize(),
+                           Flags, VT.getStoreSize(),
                            I.getAlignment() ? I.getAlignment() :
                                               DAG.getEVTAlignment(VT),
                            AAMDNodes(), nullptr, SSID, Order);
 
   InChain = TLI.prepareVolatileOrAtomicLoad(InChain, dl, DAG);
   SDValue L =
       DAG.getAtomic(ISD::ATOMIC_LOAD, dl, VT, VT, InChain,
                     getValue(I.getPointerOperand()), MMO);
void SelectionDAGBuilder::visitAtomicStore(const StoreInst &I) {
 
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   EVT VT =
       TLI.getValueType(DAG.getDataLayout(), I.getValueOperand()->getType());
 
   if (I.getAlignment() < VT.getStoreSize())
     report_fatal_error("Cannot generate unaligned atomic store");
 
-  // 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 |  MachineMemOperand::MOStore;
+  auto Flags = MachineMemOperand::MOStore;
+  if (I.isVolatile())
+    Flags |= MachineMemOperand::MOVolatile;
+  Flags |= TLI.getMMOFlags(I);
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineMemOperand *MMO =
     MF.getMachineMemOperand(MachinePointerInfo(I.getPointerOperand()), Flags,
                             VT.getStoreSize(), I.getAlignment(), AAMDNodes(),
                             nullptr, SSID, Ordering);
   SDValue OutChain =
     DAG.getAtomic(ISD::ATOMIC_STORE, dl, VT, InChain,

llvm/trunk/lib/Target/SystemZ/SystemZISelLowering.h

MachineBasicBlock *emitStringWrapper(MachineInstr &MI, MachineBasicBlock *BB,
                                        unsigned Opcode) const;
   MachineBasicBlock *emitTransactionBegin(MachineInstr &MI,
                                           MachineBasicBlock *MBB,
                                           unsigned Opcode, bool NoFloat) const;
   MachineBasicBlock *emitLoadAndTestCmp0(MachineInstr &MI,
                                          MachineBasicBlock *MBB,
                                          unsigned Opcode) const;
 
+  MachineMemOperand::Flags getMMOFlags(const Instruction &I) const override;
   const TargetRegisterClass *getRepRegClassFor(MVT VT) const override;
 };
 
 struct SystemZVectorConstantInfo {
 private:
   APInt IntBits;             // The 128 bits as an integer.
   APInt SplatBits;           // Smallest splat value.
   APInt SplatUndef;          // Bits correspoding to undef operands of the BVN.

llvm/trunk/lib/Target/SystemZ/SystemZISelLowering.cpp

SDValue Success = emitSETCC(DAG, DL, AtomicOp.getValue(1),
                               SystemZ::CCMASK_ICMP, SystemZ::CCMASK_CMP_EQ);
 
   DAG.ReplaceAllUsesOfValueWith(Op.getValue(0), AtomicOp.getValue(0));
   DAG.ReplaceAllUsesOfValueWith(Op.getValue(1), Success);
   DAG.ReplaceAllUsesOfValueWith(Op.getValue(2), AtomicOp.getValue(2));
   return SDValue();
 }
 
+MachineMemOperand::Flags
+SystemZTargetLowering::getMMOFlags(const Instruction &I) const {
+  // Because of how we convert atomic_load and atomic_store to normal loads and
+  // stores in the DAG, we need to ensure that the MMOs are marked volatile
+  // since DAGCombine hasn't been updated to account for atomic, but non
+  // volatile loads.  (See D57601)
+  if (auto *SI = dyn_cast<StoreInst>(&I))
+    if (SI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  if (auto *LI = dyn_cast<LoadInst>(&I))
+    if (LI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  if (auto *AI = dyn_cast<AtomicRMWInst>(&I))
+    if (AI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  if (auto *AI = dyn_cast<AtomicCmpXchgInst>(&I))
+    if (AI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  return MachineMemOperand::MONone;
+}
+
 SDValue SystemZTargetLowering::lowerSTACKSAVE(SDValue Op,
                                               SelectionDAG &DAG) const {
   MachineFunction &MF = DAG.getMachineFunction();
   MF.getInfo<SystemZMachineFunctionInfo>()->setManipulatesSP(true);
   return DAG.getCopyFromReg(Op.getOperand(0), SDLoc(Op),
                             SystemZ::R15D, Op.getValueType());
 }
 

llvm/trunk/lib/Target/XCore/XCoreISelLowering.h

private:
     SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINIT_TRAMPOLINE(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerADJUST_TRAMPOLINE(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerATOMIC_FENCE(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerATOMIC_LOAD(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerATOMIC_STORE(SDValue Op, SelectionDAG &DAG) const;
 
+    MachineMemOperand::Flags getMMOFlags(const Instruction &I) const override;
+
     // Inline asm support
     std::pair<unsigned, const TargetRegisterClass *>
     getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                  StringRef Constraint, MVT VT) const override;
 
     // Expand specifics
     SDValue TryExpandADDWithMul(SDNode *Op, SelectionDAG &DAG) const;
     SDValue ExpandADDSUB(SDNode *Op, SelectionDAG &DAG) const;

llvm/trunk/lib/Target/XCore/XCoreISelLowering.cpp

LowerATOMIC_STORE(SDValue Op, SelectionDAG &DAG) const {
   if (N->getMemoryVT() == MVT::i8)
     return DAG.getTruncStore(N->getChain(), SDLoc(Op), N->getVal(),
                              N->getBasePtr(), N->getPointerInfo(), MVT::i8,
                              N->getAlignment(), N->getMemOperand()->getFlags(),
                              N->getAAInfo());
   return SDValue();
 }
 
+MachineMemOperand::Flags
+XCoreTargetLowering::getMMOFlags(const Instruction &I) const {
+  // Because of how we convert atomic_load and atomic_store to normal loads and
+  // stores in the DAG, we need to ensure that the MMOs are marked volatile
+  // since DAGCombine hasn't been updated to account for atomic, but non
+  // volatile loads.  (See D57601)
+  if (auto *SI = dyn_cast<StoreInst>(&I))
+    if (SI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  if (auto *LI = dyn_cast<LoadInst>(&I))
+    if (LI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  if (auto *AI = dyn_cast<AtomicRMWInst>(&I))
+    if (AI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  if (auto *AI = dyn_cast<AtomicCmpXchgInst>(&I))
+    if (AI->isAtomic())
+      return MachineMemOperand::MOVolatile;
+  return MachineMemOperand::MONone;
+}
+
 //===----------------------------------------------------------------------===//
 //                      Calling Convention Implementation
 //===----------------------------------------------------------------------===//
 
 #include "XCoreGenCallingConv.inc"
 
 //===----------------------------------------------------------------------===//
 //                  Call Calling Convention Implementation

llvm/trunk/test/CodeGen/AMDGPU/syncscopes.ll

 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx803 -stop-after=si-insert-skips < %s | FileCheck --check-prefix=GCN %s
 
 ; GCN-LABEL: name: syncscopes
-; GCN: FLAT_STORE_DWORD killed renamable $vgpr1_vgpr2, killed renamable $vgpr0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (volatile store syncscope("agent") seq_cst 4 into %ir.agent_out)
-; GCN: FLAT_STORE_DWORD killed renamable $vgpr4_vgpr5, killed renamable $vgpr3, 0, 0, 0, implicit $exec, implicit $flat_scr :: (volatile store syncscope("workgroup") seq_cst 4 into %ir.workgroup_out)
-; GCN: FLAT_STORE_DWORD killed renamable $vgpr7_vgpr8, killed renamable $vgpr6, 0, 0, 0, implicit $exec, implicit $flat_scr :: (volatile store syncscope("wavefront") seq_cst 4 into %ir.wavefront_out)
+; GCN: FLAT_STORE_DWORD killed renamable $vgpr1_vgpr2, killed renamable $vgpr0, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store syncscope("agent") seq_cst 4 into %ir.agent_out)
+; GCN: FLAT_STORE_DWORD killed renamable $vgpr4_vgpr5, killed renamable $vgpr3, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store syncscope("workgroup") seq_cst 4 into %ir.workgroup_out)
+; GCN: FLAT_STORE_DWORD killed renamable $vgpr7_vgpr8, killed renamable $vgpr6, 0, 0, 0, implicit $exec, implicit $flat_scr :: (store syncscope("wavefront") seq_cst 4 into %ir.wavefront_out)
 define void @syncscopes(
     i32 %agent,
     i32* %agent_out,
     i32 %workgroup,
     i32* %workgroup_out,
     i32 %wavefront,
     i32* %wavefront_out) {
 entry:
   store atomic i32 %agent, i32* %agent_out syncscope("agent") seq_cst, align 4
   store atomic i32 %workgroup, i32* %workgroup_out syncscope("workgroup") seq_cst, align 4
   store atomic i32 %wavefront, i32* %wavefront_out syncscope("wavefront") seq_cst, align 4
   ret void
 }