Skip to content

[AA] A conservative fix for atomic store instruction. #155032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[AA] A conservative fix for atomic store instruction. #155032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions llvm/lib/Analysis/AliasAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@

using namespace llvm;

STATISTIC(NumNoAlias, "Number of NoAlias results");
STATISTIC(NumMayAlias, "Number of MayAlias results");
STATISTIC(NumNoAlias, "Number of NoAlias results");
STATISTIC(NumMayAlias, "Number of MayAlias results");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

STATISTIC(NumMustAlias, "Number of MustAlias results");

/// Allow disabling BasicAA from the AA results. This is particularly useful
Expand Down Expand Up @@ -118,8 +118,8 @@ AliasResult AAResults::alias(const MemoryLocation &LocA,
if (EnableAATrace) {
for (unsigned I = 0; I < AAQI.Depth; ++I)
dbgs() << " ";
dbgs() << "Start " << *LocA.Ptr << " @ " << LocA.Size << ", "
<< *LocB.Ptr << " @ " << LocB.Size << "\n";
dbgs() << "Start " << *LocA.Ptr << " @ " << LocA.Size << ", " << *LocB.Ptr
<< " @ " << LocB.Size << "\n";
}

AAQI.Depth++;
Expand All @@ -133,8 +133,8 @@ AliasResult AAResults::alias(const MemoryLocation &LocA,
if (EnableAATrace) {
for (unsigned I = 0; I < AAQI.Depth; ++I)
dbgs() << " ";
dbgs() << "End " << *LocA.Ptr << " @ " << LocA.Size << ", "
<< *LocB.Ptr << " @ " << LocB.Size << " = " << Result << "\n";
dbgs() << "End " << *LocA.Ptr << " @ " << LocA.Size << ", " << *LocB.Ptr
<< " @ " << LocB.Size << " = " << Result << "\n";
}

if (AAQI.Depth == 0) {
Expand Down Expand Up @@ -421,9 +421,18 @@ ModRefInfo AAResults::getModRefInfo(const LoadInst *L,
const MemoryLocation &Loc,
AAQueryInfo &AAQI) {
// Be conservative in the face of atomic.
if (isStrongerThan(L->getOrdering(), AtomicOrdering::Unordered))
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return ModRefInfo::ModRef;
}

// If the load address doesn't alias the given address, it doesn't read
// or write the specified memory.
if (Loc.Ptr) {
Expand All @@ -439,7 +448,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) {
Expand Down Expand Up @@ -538,7 +547,8 @@ ModRefInfo AAResults::getModRefInfo(const AtomicCmpXchgInst *CX,
ModRefInfo AAResults::getModRefInfo(const AtomicRMWInst *RMW,
const MemoryLocation &Loc,
AAQueryInfo &AAQI) {
// Acquire/Release atomicrmw has properties that matter for arbitrary addresses.
// Acquire/Release atomicrmw has properties that matter for arbitrary
// addresses.
if (isStrongerThanMonotonic(RMW->getOrdering()))
return ModRefInfo::ModRef;

Expand Down Expand Up @@ -600,8 +610,7 @@ ModRefInfo AAResults::getModRefInfo(const Instruction *I,
/// with a smarter AA in place, this test is just wasting compile time.
ModRefInfo AAResults::callCapturesBefore(const Instruction *I,
const MemoryLocation &MemLoc,
DominatorTree *DT,
AAQueryInfo &AAQI) {
DominatorTree *DT, AAQueryInfo &AAQI) {
if (!DT)
return ModRefInfo::ModRef;

Expand Down Expand Up @@ -677,7 +686,7 @@ bool AAResults::canInstructionRangeModRef(const Instruction &I1,
"Instructions not in same basic block!");
BasicBlock::const_iterator I = I1.getIterator();
BasicBlock::const_iterator E = I2.getIterator();
++E; // Convert from inclusive to exclusive range.
++E; // Convert from inclusive to exclusive range.

for (; I != E; ++I) // Check every instruction in range
if (isModOrRefSet(getModRefInfo(&*I, Loc) & Mode))
Expand Down
99 changes: 88 additions & 11 deletions llvm/unittests/Analysis/AliasAnalysisTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ using namespace llvm;

// Set up some test passes.
namespace llvm {
void initializeAATestPassPass(PassRegistry&);
void initializeTestCustomAAWrapperPassPass(PassRegistry&);
}
void initializeAATestPassPass(PassRegistry &);
void initializeTestCustomAAWrapperPassPass(PassRegistry &);
} // namespace llvm

namespace {
struct AATestPass : FunctionPass {
Expand Down Expand Up @@ -61,7 +61,7 @@ struct AATestPass : FunctionPass {
return false;
}
};
}
} // namespace

char AATestPass::ID = 0;
INITIALIZE_PASS_BEGIN(AATestPass, "aa-test-pas", "Alias Analysis Test Pass",
Expand Down Expand Up @@ -90,7 +90,7 @@ struct TestCustomAAResult : AAResultBase {
return AliasResult::MayAlias;
}
};
}
} // namespace

namespace {
/// A wrapper pass for the legacy pass manager to use with the above custom AA
Expand Down Expand Up @@ -126,14 +126,14 @@ class TestCustomAAWrapperPass : public ImmutablePass {
TestCustomAAResult &getResult() { return *Result; }
const TestCustomAAResult &getResult() const { return *Result; }
};
}
} // namespace

char TestCustomAAWrapperPass::ID = 0;
INITIALIZE_PASS_BEGIN(TestCustomAAWrapperPass, "test-custom-aa",
"Test Custom AA Wrapper Pass", false, true)
"Test Custom AA Wrapper Pass", false, true)
INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
INITIALIZE_PASS_END(TestCustomAAWrapperPass, "test-custom-aa",
"Test Custom AA Wrapper Pass", false, true)
"Test Custom AA Wrapper Pass", false, true)

namespace {

Expand Down Expand Up @@ -246,7 +246,8 @@ TEST_F(AliasAnalysisTest, BatchAAPhiCycles) {
%s2 = select i1 %c, i8* %a2, i8* %a1
br label %loop
}
)", Err, C);
)",
Err, C);

Function *F = M->getFunction("f");
Instruction *Phi = getInstructionByName(*F, "phi");
Expand Down Expand Up @@ -291,7 +292,8 @@ TEST_F(AliasAnalysisTest, BatchAAPhiAssumption) {
%b.next = getelementptr i8, i8* %b, i64 1
br label %loop
}
)", Err, C);
)",
Err, C);

Function *F = M->getFunction("f");
Instruction *A = getInstructionByName(*F, "a");
Expand Down Expand Up @@ -425,4 +427,79 @@ TEST_F(AAPassInfraTest, injectExternalAA) {
EXPECT_TRUE(IsCustomAAQueried);
}

} // end anonymous namspace
// Test that a monotonic atomic load is treated as ModRef ONLY against a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a LIT test instead?

// MustAlias locations, and as Ref against a MayAlias location.
TEST_F(AliasAnalysisTest, MonotonicAtomicLoadIsModRefOnlyForMustAlias) {
LLVMContext C;
SMDiagnostic Err;
std::unique_ptr<Module> M = parseAssemblyString(R"(
define void @atomic_load_modref_test(i8* %p1, i8* %p2) {
entry:
%val = load atomic i8, i8* %p1 monotonic, align 1
ret void
}
)",
Err, C);

ASSERT_TRUE(M);

Function *F = M->getFunction("atomic_load_modref_test");
const Instruction *AtomicLoad = &F->getEntryBlock().front();
const Value *Ptr1 = F->getArg(0);
const Value *Ptr2 = F->getArg(1);

MemoryLocation Ptr1Loc = MemoryLocation(Ptr1, LocationSize::precise(1));
MemoryLocation Ptr2Loc = MemoryLocation(Ptr2, LocationSize::precise(1));

auto &AA = getAAResults(*F);

// 1. MustAlias Case: The atomic load on %p1 against a location on %p1.
// This should trigger the new logic and return ModRef.
EXPECT_EQ(ModRefInfo::ModRef, AA.getModRefInfo(AtomicLoad, Ptr1Loc));

// 2. MayAlias Case: The atomic load on %p1 against a location on %p2.
// This should NOT trigger the new logic and should fall back to returning
// Ref.
EXPECT_EQ(ModRefInfo::Ref, AA.getModRefInfo(AtomicLoad, Ptr2Loc));
}

// Test that a monotonic atomic store is no longer considered "strong" and
// falls through to the alias checking logic, while stroner orderings do not.
TEST_F(AliasAnalysisTest, MonotonicAtomicStoreAliasBehavior) {
LLVMContext C;
SMDiagnostic Err;
std::unique_ptr<Module> M = parseAssemblyString(R"(
define void @atomic_store_test(i8* noalias %p1, i8* noalias %p2) {
entry:
store atomic i8 0, i8* %p1 monotonic, align 1
store atomic i8 0, i8* %p1 release, align 1
ret void
}
)",
Err, C);

ASSERT_TRUE(M);

Function *F = M->getFunction("atomic_store_test");
auto It = F->getEntryBlock().begin();
const Instruction *MonotonicStore = &*It++;
const Instruction *ReleaseStore = &*It;

const Value *Ptr2 = F->getArg(1);
MemoryLocation Ptr2Loc = MemoryLocation(Ptr2, LocationSize::precise(1));

auto &AA = getAAResults(*F);

// 1. Test the Monotonic store.
// With the change, the Monotonic store should go through the
// isStrongerThan() check. Since %p1 and %p2 are noalias, the alias check
// should return NoAlias, resulting in NoModRef.
EXPECT_EQ(ModRefInfo::NoModRef, AA.getModRefInfo(MonotonicStore, Ptr2Loc));

// 2. Test the Relase (stronger) store.
// The release atomic store should be caught by the isStrongerThan() check.
// return ModRef without performing an alias check.
EXPECT_EQ(ModRefInfo::ModRef, AA.getModRefInfo(ReleaseStore, Ptr2Loc));
}

} // namespace
Loading