[CodeGen] Be conservative about atomic accesses as for volatile

[reames]

Summary

Background: At the moment, we record the AtomicOrdering of an access in the MMO, but also mark any atomic access as volatile in SelectionDAG. I'm working towards separating that. See https://reviews.llvm.org/D57601 for context.

Update all usages of isVolatile in lib/CodeGen to preserve behaviour once atomic MMOs stop being also volatile. This is NFC in it's current form, but is essential for correctness once we make that final change.

It useful to keep in mind that AtomicSDNode is not a parent of LoadSDNode, StoreSDNode, or LSBaseSDNode. As a result, any call to isVolatile on one of those static types doesn't need a companion isAtomic check.

I'm deliberately being conservative about handling. I want the change to stop adding volatile to be NFC itself, and then will work through places where we can be less conservative for atomics one by one in separate changes w/tests.

[jlebar]

My main comment is, in most or all of these places where we s/isVolatile/isVolatile && isAtomic/, it's not going to be obvious to a reader whether isAtomic is really necessary, or just part of a conservative check.

I see that you plan to go back over these, and that's cool, but I wonder if we should leave a breadcrumb that indicates that you plan to come back to this. That way if you miss a spot or whatever, readers won't think that isAtomic is somehow deeply meaningful (and won't have to scour through blame to find this comment I'm leaving now... :).

Even something as simple as //TODO(reames): Figure out whether isAtomic is really necessary (see D57601).

include/llvm/CodeGen/SelectionDAGNodes.h

69

Nit: Is this comment worth the cost?

[reames]

In D57596#1381092, @jlebar wrote:

My main comment is, in most or all of these places where we s/isVolatile/isVolatile && isAtomic/, it's not going to be obvious to a reader whether isAtomic is really necessary, or just part of a conservative check.

I see that you plan to go back over these, and that's cool, but I wonder if we should leave a breadcrumb that indicates that you plan to come back to this. That way if you miss a spot or whatever, readers won't think that isAtomic is somehow deeply meaningful (and won't have to scour through blame to find this comment I'm leaving now... :).

Even something as simple as //TODO(reames): Figure out whether isAtomic is really necessary (see D57601).

Sure. I won't include the name since that's not idiomatic, but I'll add comments and update.

[reames]

Update w/requested todos and a simplification. It occurred to me that LoadSDNode is not a subclass of AtomicSDNode, so anything of static type LoadSDNode doesn't need the conservative atomicity check.

include/llvm/CodeGen/SelectionDAGNodes.h

 class MachineBasicBlock;
 class MachineConstantPoolValue;
 class MCSymbol;
 class raw_ostream;
 class SDNode;
 class SelectionDAG;
 class Type;
 class Value;
+class AtomicSDNode; //forward declare for type check
 
 void checkForCycles(const SDNode *N, const SelectionDAG *DAG = nullptr,
                     bool force = false);
 
 /// This represents a list of ValueType's that has been intern'd by
 /// a SelectionDAG.  Instances of this simple value class are returned by
 /// SelectionDAG::getVTList(...).
 ///
unsigned getRawSubclassData() const {
     };
     memcpy(&RawSDNodeBits, &this->RawSDNodeBits, sizeof(this->RawSDNodeBits));
     SDNodeBits.HasDebugValue = 0;
     SDNodeBits.IsDivergent = false;
     memcpy(&Data, &RawSDNodeBits, sizeof(RawSDNodeBits));
     return Data;
   }
 
+  bool isAtomic() const { return isa<AtomicSDNode>(this); }
   bool isVolatile() const { return MemSDNodeBits.IsVolatile; }
   bool isNonTemporal() const { return MemSDNodeBits.IsNonTemporal; }
   bool isDereferenceable() const { return MemSDNodeBits.IsDereferenceable; }
   bool isInvariant() const { return MemSDNodeBits.IsInvariant; }
 
   // Returns the offset from the location of the access.
   int64_t getSrcValueOffset() const { return MMO->getOffset(); }
 

lib/CodeGen/MachineInstr.cpp

bool MachineInstr::isDereferenceableInvariantLoad(AliasAnalysis *AA) const {
   // it may not be an invariant load.
   if (memoperands_empty())
     return false;
 
   const MachineFrameInfo &MFI = getParent()->getParent()->getFrameInfo();
 
   for (MachineMemOperand *MMO : memoperands()) {
     if (MMO->isVolatile()) return false;
+    if (MMO->isAtomic()) return false;
     if (MMO->isStore()) return false;
     if (MMO->isInvariant() && MMO->isDereferenceable())
       continue;
 
     // A load from a constant PseudoSourceValue is invariant.
     if (const PseudoSourceValue *PSV = MMO->getPseudoValue())
       if (PSV->isConstant(&MFI))
         continue;

lib/CodeGen/MachinePipeliner.cpp

void SwingSchedulerDAG::updateMemOperands(MachineInstr &NewMI,
   if (Num == 0)
     return;
   // If the instruction has memory operands, then adjust the offset
   // when the instruction appears in different stages.
   if (NewMI.memoperands_empty())
     return;
   SmallVector<MachineMemOperand *, 2> NewMMOs;
   for (MachineMemOperand *MMO : NewMI.memoperands()) {
-    if (MMO->isVolatile() || (MMO->isInvariant() && MMO->isDereferenceable()) ||
+    if (MMO->isVolatile() || MMO->isAtomic() ||
+        (MMO->isInvariant() && MMO->isDereferenceable()) ||
         (!MMO->getValue())) {
       NewMMOs.push_back(MMO);
       continue;
     }
     unsigned Delta;
     if (Num != UINT_MAX && computeDelta(OldMI, Delta)) {
       int64_t AdjOffset = Delta * Num;
       NewMMOs.push_back(

lib/CodeGen/ScheduleDAGInstrs.cpp

 /// for that object. This function returns false the memory location is
 /// unknown or may alias anything.
 static bool getUnderlyingObjectsForInstr(const MachineInstr *MI,
                                          const MachineFrameInfo &MFI,
                                          UnderlyingObjectsVector &Objects,
                                          const DataLayout &DL) {
   auto allMMOsOkay = [&]() {
     for (const MachineMemOperand *MMO : MI->memoperands()) {
-      if (MMO->isVolatile())
+      if (MMO->isVolatile() || MMO->isAtomic())
         return false;
 
       if (const PseudoSourceValue *PSV = MMO->getPseudoValue()) {
         // Function that contain tail calls don't have unique PseudoSourceValue
         // objects. Two PseudoSourceValues might refer to the same or
         // overlapping locations. The client code calling this function assumes
         // this is not the case. So return a conservative answer of no known
         // object.

lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp

SDValue DAGTypeLegalizer::GenWidenVectorLoads(SmallVectorImpl<SDValue> &LdChain,
   SDValue Chain = LD->getChain();
   SDValue BasePtr = LD->getBasePtr();
   unsigned Align = LD->getAlignment();
   MachineMemOperand::Flags MMOFlags = LD->getMemOperand()->getFlags();
   AAMDNodes AAInfo = LD->getAAInfo();
 
   int LdWidth = LdVT.getSizeInBits();
   int WidthDiff = WidenWidth - LdWidth;
-  unsigned LdAlign = LD->isVolatile() ? 0 : Align; // Allow wider loads.
+  // Allow wider loads.
+  unsigned LdAlign = (LD->isVolatile() || LD->isAtomic()) ? 0 : Align;
 
   // Find the vector type that can load from.
   EVT NewVT = FindMemType(DAG, TLI, LdWidth, WidenVT, LdAlign, WidthDiff);
   int NewVTWidth = NewVT.getSizeInBits();
   SDValue LdOp = DAG.getLoad(NewVT, dl, Chain, BasePtr, LD->getPointerInfo(),
                              Align, MMOFlags, AAInfo);
   LdChain.push_back(LdOp.getValue(1));
 

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (getOpcode() == ISD::TokenFactor) {
     // reaches Dest.
     return llvm::all_of((*this)->ops(), [=](SDValue Op) {
       return Op.reachesChainWithoutSideEffects(Dest, Depth - 1);
     });
   }
 
   // Loads don't have side effects, look through them.
   if (LoadSDNode *Ld = dyn_cast<LoadSDNode>(*this)) {
-    if (!Ld->isVolatile())
+    if (!Ld->isVolatile() && !Ld->isAtomic())
       return Ld->getChain().reachesChainWithoutSideEffects(Dest, Depth-1);
   }
   return false;
 }
 
 bool SDNode::hasPredecessor(const SDNode *N) const {
   SmallPtrSet<const SDNode *, 32> Visited;
   SmallVector<const SDNode *, 16> Worklist;

 }
 
 bool SelectionDAG::areNonVolatileConsecutiveLoads(LoadSDNode *LD,
                                                   LoadSDNode *Base,
                                                   unsigned Bytes,
                                                   int Dist) const {
   if (LD->isVolatile() || Base->isVolatile())
     return false;
+  if (LD->isAtomic() || Base->isAtomic())
+    return false;
   if (LD->isIndexed() || Base->isIndexed())
     return false;
   if (LD->getChain() != Base->getChain())
     return false;
   EVT VT = LD->getValueType(0);
   if (VT.getSizeInBits() / 8 != Bytes)
     return false;
 

lib/CodeGen/SelectionDAG/TargetLowering.cpp

if (DCI.isBeforeLegalize() &&
         N0.getOpcode() == ISD::AND && C1 == 0 &&
         N0.getNode()->hasOneUse() &&
         isa<LoadSDNode>(N0.getOperand(0)) &&
         N0.getOperand(0).getNode()->hasOneUse() &&
         isa<ConstantSDNode>(N0.getOperand(1))) {
       LoadSDNode *Lod = cast<LoadSDNode>(N0.getOperand(0));
       APInt bestMask;
       unsigned bestWidth = 0, bestOffset = 0;
-      if (!Lod->isVolatile() && Lod->isUnindexed()) {
+      if (!Lod->isVolatile() && !Lod->isAtomic() && Lod->isUnindexed()) {
         unsigned origWidth = N0.getValueSizeInBits();
         unsigned maskWidth = origWidth;
         // We can narrow (e.g.) 16-bit extending loads on 32-bit target to
         // 8 bits, but have to be careful...
         if (Lod->getExtensionType() != ISD::NON_EXTLOAD)
           origWidth = Lod->getMemoryVT().getSizeInBits();
         const APInt &Mask =
           cast<ConstantSDNode>(N0.getOperand(1))->getAPIntValue();