Allow ImproveChain to get past relaxed atomics

[yoni.lavi]

Summary

As titled; Relaxed AKA Monotonic atomics don't impose any "global" synchronization semantics between threads.
Any other non-aliasing mem operation can treat them the same as an ordinary load (or load/modify/store) so it should not preclude chain improvement.

The case of aliasing load being reordered around an atomic load of the same deserves mention here:

• Technically, reordering changes program semantics, in the sense that:
  • loading x after atomically loading x, the non-atomic load cannot give us a result that is more stale than the atomic one
  • loading x before atomically loading x, the non-atomic load can result in an arbitrarily stale value
• We still allow this, on the grounds that:
  • If nothing is concurrently modifying x, there is no difference.
  • If any concurrent writers to x potentially exist, both versions of the program suffer from UB due to a data race, so it is legit to alter semantics in this manner.

[yoni-lavi]

• lint fixes

[yoni-lavi]

• Fixed getOrdering() -> getMergedOrdering()

[yoni-lavi]

• Emptying my diff just to check test stability

[yoni-lavi]

• Restore original

[yoni-lavi]

I've confirmed my diff does not cause any test failures that aren't already present in the base revision (latest main).

It does cause ThreadSanitizer-x86_64 :: restore_stack.cpp to time out, Though test may well be flaky in that regard; it also doesn't contain any relaxed atomics as far as I can tell.

[yoni-lavi]

• rb to re-test

[yoni-lavi]

• removing all content to check for flaky test timeouts again

[yoni-lavi]

• again

[yoni-lavi]

restored content, without debug prints

[yoni-lavi]

ping

[RKSimon]

In D121366#3374969, @yoni-lavi wrote:

I've confirmed my diff does not cause any test failures that aren't already present in the base revision (latest main).

Do you have tests that you can add that do show a change?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

24071

is this how clang-format formats the code?

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

24066

I'm not sure what this comment is trying to say

[yoni-lavi]

I can definitely come up with a test case where the SDAG is different.

However, strictly speaking, weakening the chain dependencies doesn't ever "force" a change in the output instruction schedule.

So a good test would have to assert on the SDAG structure rather than the output schedule / machine instructions.

Is there an existing framework to assert on the SDAG for a given basic block in an example program?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

24066

It's related to the case above (LSBaseSDNode) where Offset is computed to account for PRE_INC / PRE_DEC.

If I understood it right, this way of representing ptr increments as part of the memory access itself does not exist for the "atomic" SDNodes. Hence the Offset = 0 and this comment.

24071

yes. (the phabricator build if clang-format wants to re-format your code...)

[yoni-lavi]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

24071

no idea why editing isn't allowed ... but, I meant to write: "the phabricator build fails if ..."

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

24066

The use "allegedly" and "issue" make it seem like there might be some like might be some problem or question here. Can you write it in a more confident way. Like "Atomic instructions don't have an offset"

[yoni.lavi]

Created new account here, bound to my github account.
I will also rebase this and attempt to add tests.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (const auto *LSN = dyn_cast<LSBaseSDNode>(N)) {
                            : 0;
       uint64_t Size =
           MemoryLocation::getSizeOrUnknown(LSN->getMemoryVT().getStoreSize());
       return {LSN->isVolatile(), LSN->isAtomic(), LSN->getBasePtr(),
               Offset /*base offset*/,
               Optional<int64_t>(Size),
               LSN->getMemOperand()};
     }
+    if (const auto *AN = dyn_cast<AtomicSDNode>(N)) {
+      // PRE_INC / PRE_DEC allegedly not a relevant issue, can set this to zero
+      int64_t Offset = 0;
+      uint64_t Size =
+          MemoryLocation::getSizeOrUnknown(AN->getMemoryVT().getStoreSize());
+      return {AN->isVolatile(), AN->isAtomic(),          AN->getBasePtr(),
+              Offset,           Optional<int64_t>(Size), AN->getMemOperand()};
+    }
     if (const auto *LN = cast<LifetimeSDNode>(N))
       return {false /*isVolatile*/, /*isAtomic*/ false, LN->getOperand(1),
               (LN->hasOffset()) ? LN->getOffset() : 0,
               (LN->hasOffset()) ? Optional<int64_t>(LN->getSize())
                                 : Optional<int64_t>(),
               (MachineMemOperand *)nullptr};
     // Default.
     return {false /*isvolatile*/, /*isAtomic*/ false, SDValue(),
case ISD::LIFETIME_END: {
       if (!mayAlias(N, C.getNode())) {
         // Look further up the chain.
         C = C.getOperand(0);
         return true;
       }
       return false;
     }
     default:
+      if (auto *AC = dyn_cast<AtomicSDNode>(C.getNode())) {
+        if (AC->getMergedOrdering() == AtomicOrdering::Monotonic) {
+          bool IsOpLoad = AC->getOpcode() == ISD::ATOMIC_LOAD;
+          if ((IsLoad && IsOpLoad) || !mayAlias(N, AC)) {
+            C = C.getOperand(0);
+            return true;
+          }
+        }
+      }
       return false;
     }
   };
 
   // Look at each chain and determine if it is an alias.  If so, add it to the
   // aliases list.  If not, then continue up the chain looking for the next
   // candidate.
   while (!Chains.empty()) {