[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 ↗

(On Diff #184774)

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.

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;
+    // TODO: Figure out whether isAtomic is really necessary (see D57601).
+    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()) ||
+    // TODO: Figure out whether isAtomic is really necessary (see D57601).
+    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())
+      // TODO: Figure out whether isAtomic is really necessary (see D57601).
+      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.