[AA] A conservative fix for atomic store instruction.

[jinhuang1102]

The current atomic store check in function AAResults::getModRefInfo would compare to the AtomicOrdering::Unordered that mean any atomic store instruction will return ModRefInfo::ModRef without performing any further analysis on the memory location itself.

This PR raises the comparison form Unordered to Monotonic.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Jin Huang (jinhuang1102)

Changes

The current atomic store check in function AAResults::getModRefInfo would compare to the AtomicOrdering::Unordered that mean any atomic store instruction will return ModRefInfo::ModRef without performing any further analysis on the memory location itself.

This PR raises the comparison form Unordered to Monotonic.

---

Full diff: https://github.com/llvm/llvm-project/pull/155032.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/AliasAnalysis.cpp (+1-1)

diff --git a/llvm/lib/Analysis/AliasAnalysis.cpp b/llvm/lib/Analysis/AliasAnalysis.cpp
index 3ec009ca4adde..b18ef88509b5c 100644
--- a/llvm/lib/Analysis/AliasAnalysis.cpp
+++ b/llvm/lib/Analysis/AliasAnalysis.cpp
@@ -439,7 +439,7 @@ ModRefInfo AAResults::getModRefInfo(const StoreInst *S,
                                     const MemoryLocation &Loc,
                                     AAQueryInfo &AAQI) {
   // Be conservative in the face of atomic.
-  if (isStrongerThan(S->getOrdering(), AtomicOrdering::Unordered))
+  if (isStrongerThan(S->getOrdering(), AtomicOrdering::Monotonic))
     return ModRefInfo::ModRef;
 
   if (Loc.Ptr) {

[david-xl]

Please add a test case.

[nikic]

I think the general idea here is right. Relaxed store should not modref other locations.

If we change this, I think we should change load and store at the same time. I think we should also make sure that we still return ModRef for relaxed, just not for unrelated locations. I believe this is definitely necessary for loads (otherwise we'd no longer encode an ordering in the memory effects, which would need much larger changes). I can't immediately think of a case that would break if we only return Mod for stores, but that's probably my lack of imagination.

[david-xl]

yes, the same fix should be applied to LoadInst too (with a test case). @nikic can you clarify 'return ModRef for relaxed?'

The fix can probably be expanded to handle stronger constraints. For instance if the store is 'seq_cst', it imposes order on seq_cst loads of all addresses but not on non seq_cst loads. To handle this, the interface will need to be enhanced to pass the instruction instead of MemoryLocation.

[efriedma-quic]

A seq_cst store is a release barrier, and therefore does impose constraints on non-seq_cst loads.

That isn't to say all code motion is illegal, but you'd need an interface that's much more specialized than getModRefInfo. And I'm not sure it's worth inventing a new interface here without a specific use-case in mind.

[david-xl]

A seq_cst store is a release barrier, and therefore does impose constraints on non-seq_cst loads.

That isn't to say all code motion is illegal, but you'd need an interface that's much more specialized than getModRefInfo. And I'm not sure it's worth inventing a new interface here without a specific use-case in mind.

+1

[nikic]

yes, the same fix should be applied to LoadInst too (with a test case). @nikic can you clarify 'return ModRef for relaxed?'

Previously we returned ModRef for non-aliasing locations and for aliasing locations. This patch will return NoModRef for non-aliasing locations and Mod for aliasing locations. I'm suggesting NoModRef for non-aliasing locations and ModRef for aliasing locations. ModRef encodes the ordering requirement for relaxed/monotonic operations.

(As said, I'm not 100% sure just Mod is wrong for store, just that Ref would be wrong for monotonic load, and I'd rather keep symmetry.)

[david-xl]

yes, the same fix should be applied to LoadInst too (with a test case). @nikic can you clarify 'return ModRef for relaxed?'

Previously we returned ModRef for non-aliasing locations and for aliasing locations. This patch will return NoModRef for non-aliasing locations and Mod for aliasing locations. I'm suggesting NoModRef for non-aliasing locations and ModRef for aliasing locations. ModRef encodes the ordering requirement for relaxed/monotonic operations.

(As said, I'm not 100% sure just Mod is wrong for store, just that Ref would be wrong for monotonic load, and I'd rather keep symmetry.)

This makes total sense (limiting the fix for one scenario -- non-aliasing relaxed loads/stores).

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jinhuang1102]

Hi, I am back from vacation and have just pushed some updates to address the feedback from the previous review. The main functional change is in llvm/lib/Analysis/AliasAnalysis.cpp to refine the logic for atomic load instructions see below.

  // Be conservative in the face of atomic.
  if (isStrongerThan(L->getOrdering(), AtomicOrdering::Monotonic))
    return ModRefInfo::ModRef;

  // For Monotonic and unordered loads, we only need to be more conservative
  // when the locations are MustAlias to prevent unsafe reordering of accesses
  // to the same memory. For MayAlias, we can treat it as a normal read.
  if (L->isAtomic()) {
    if (Loc.Ptr &&
        alias(MemoryLocation::get(L), Loc, AAQI, L) == AliasResult::MustAlias)
      return ModRefInfo::ModRef;
  }

Two new tests (MonotonicAtomicLoadIsModRefOnlyForMustAlias and MonotonicAtomicStoreAliasBehavior) are also added to AliasAnalysisTest.cpp.

The only functional changes are in these two files; other modifications are NFC (No Functional Change) from clang-format.

[david-xl]

Should be done conservatively for MayAlias too -- that means the check should be done at line 433 below.

[david-xl]

The suggestion to also return ModRef for the store case (for symmetry).

[nikic]

Basically, what I'd like to see is that the return ModRefInfo::Ref; below gets replaced with return IsStrongerThanUnordered ? ModRefInfo::ModRef : ModRefInfo::Ref. The rest of the code shouldn't need to change.

[jinhuang1102]

Regarding atomic loads, I believe the correct behavior is to return ModRef, except in cases where the locations are proven to be NoAlias for the atomic load instruction.

My reasoning is that of the four possible alias analysis results only NoAlias convert to false for a binary flag indicating whether there is the possibility of aliasing.

[david-xl]

Can you add a LIT test instead?

[nikic]

Please don't reformat unrelated code. You can use git-clang-format to only format the changed lines.

[jinhuang1102]

To make the review process easier, I will create a new, cleaner pull request. The new PR contains only the functional changes and has LIT test instead of unit tests.

[jinhuang1102]

#158169 is for functional changes only. Please submit for review, this PR has expired.