Skip to content

[FlattenCFG] Fix an Imprecise Usage of AA #128117

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

Merged
merged 13 commits into from
Apr 18, 2025
Merged

[FlattenCFG] Fix an Imprecise Usage of AA #128117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2d987ba
Fix a wrong use of AA in FlattenCFG
Chengjunp Feb 21, 2025
5cd91b8
Update test
Chengjunp Feb 21, 2025
cc3b893
Support check alias between two instructions using getModRefInfo
Chengjunp Feb 22, 2025
68c4c7b
Update testcase
Chengjunp Feb 22, 2025
ee15a70
Update the comments of the test
Chengjunp Feb 22, 2025
12293ce
Add a unit test IsNoAliasForInstructions
Chengjunp Feb 25, 2025
28dfc43
Format
Chengjunp Feb 25, 2025
6f3217a
Add a new helper getModRefInfo for two Instructions and update
Chengjunp Feb 26, 2025
326c5de
Add helper
Chengjunp Apr 15, 2025
137257d
Merge branch 'main' into chengjunp/FlattenCFG_AA_use_fix
Chengjunp Apr 17, 2025
2d000dc
Add FIXME
Chengjunp Apr 17, 2025
6cb7208
Format
Chengjunp Apr 17, 2025
2e93b5c
Merge branch 'chengjunp/FlattenCFG_AA_use_fix' of github.com:Chengjun…
Chengjunp Apr 17, 2025
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
6 changes: 6 additions & 0 deletions llvm/include/llvm/Analysis/AliasAnalysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,10 @@ class AAResults {
/// the same memory locations.
ModRefInfo getModRefInfo(const Instruction *I, const CallBase *Call);

/// Return information about whether two instructions may refer to the same
/// memory locations.
ModRefInfo getModRefInfo(const Instruction *I1, const Instruction *I2);

/// Return information about whether a particular call site modifies
/// or reads the specified memory location \p MemLoc before instruction \p I
/// in a BasicBlock.
Expand Down Expand Up @@ -600,6 +604,8 @@ class AAResults {
ModRefInfo getModRefInfo(const Instruction *I,
const std::optional<MemoryLocation> &OptLoc,
AAQueryInfo &AAQIP);
ModRefInfo getModRefInfo(const Instruction *I1, const Instruction *I2,
AAQueryInfo &AAQI);
ModRefInfo callCapturesBefore(const Instruction *I,
const MemoryLocation &MemLoc, DominatorTree *DT,
AAQueryInfo &AAQIP);
Expand Down
20 changes: 20 additions & 0 deletions llvm/lib/Analysis/AliasAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,26 @@ ModRefInfo AAResults::getModRefInfo(const CallBase *Call1,
return Result;
}

ModRefInfo AAResults::getModRefInfo(const Instruction *I1,
const Instruction *I2) {
SimpleAAQueryInfo AAQIP(*this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should accept AAQueryInfo as parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify this bit: We should have a method taking AAQI, and another helper that uses SimplieAAQueryInfo and call the main method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the methods. Let me know whether it is you expected.

return getModRefInfo(I1, I2, AAQIP);
}

ModRefInfo AAResults::getModRefInfo(const Instruction *I1,
const Instruction *I2, AAQueryInfo &AAQI) {
// Early-exit if either instruction does not read or write memory.
if (!I1->mayReadOrWriteMemory() || !I2->mayReadOrWriteMemory())
return ModRefInfo::NoModRef;

if (const auto *Call2 = dyn_cast<CallBase>(I2))
return getModRefInfo(I1, Call2, AAQI);

// FIXME: We can have a more precise result.
ModRefInfo MR = getModRefInfo(I1, MemoryLocation::getOrNone(I2), AAQI);
return isModOrRefSet(MR) ? ModRefInfo::ModRef : ModRefInfo::NoModRef;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be more precise, but it's okay to start with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be a better / more precise way to check the ModRefInfo? Thanks!

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 you are currently doing is a very crude check that the two instructions somehow access the same memory. It can be made more precise by checking the kind of access. For example, if both instructions only read memory, we can actually return NoModRef, because read-read dependences can be ignored. I guess you could easily handle that part by updating your mayReadOrWriteMemory checks to check that at least one is a write instead.

Then if I1 is a read (and I2 a write) we can return Ref instead. If I1 is write (and I1 a read) we can return Mod. (This is why the order of the argument matters.)

Basically, what the Call-Call handling does starting from:

llvm-project/llvm/lib/Analysis/AliasAnalysis.cpp

Line 289 in 326c5de

// If they both only read from memory, there is no dependence.

I think it's ok if you add a FIXME.

}

MemoryEffects AAResults::getMemoryEffects(const CallBase *Call,
AAQueryInfo &AAQI) {
MemoryEffects Result = MemoryEffects::unknown();
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Transforms/Utils/FlattenCFG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,8 @@ bool FlattenCFGOpt::CompareIfRegionBlock(BasicBlock *Block1, BasicBlock *Block2,
if (iter1->mayWriteToMemory()) {
for (BasicBlock::iterator BI(PBI2), BE(PTI2); BI != BE; ++BI) {
if (BI->mayReadFromMemory() || BI->mayWriteToMemory()) {
// Check alias with Head2.
if (!AA || !AA->isNoAlias(&*iter1, &*BI))
// Check whether iter1 and BI may access the same memory location.
if (!AA || AA->getModRefInfo(&*iter1, &*BI) != ModRefInfo::NoModRef)
return false;
}
}
Expand Down
140 changes: 140 additions & 0 deletions llvm/test/Transforms/Util/flatten-cfg.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,3 +309,143 @@ if.then.y:
exit:
ret i1 %cmp.y
}

declare void @foo()
declare void @bar() #1

; Test that two if-regions are not merged when there's potential aliasing
; between a store in the first if-region and a load in the second if-region's header
define i32 @test_alias(i32 %a, i32 %b, ptr %p1, ptr %p2) {
; CHECK-LABEL: define i32 @test_alias
; CHECK-SAME: (i32 [[A:%.*]], i32 [[B:%.*]], ptr [[P1:%.*]], ptr [[P2:%.*]]) {
; CHECK-NEXT: entry:
; CHECK-NEXT: store i32 42, ptr [[P1]], align 4
; CHECK-NEXT: [[COND1:%.*]] = icmp eq i32 [[A]], 0
; CHECK-NEXT: br i1 [[COND1]], label [[IF_THEN1:%.*]], label [[IF_END1:%.*]]
; CHECK: if.then1:
; CHECK-NEXT: store i32 100, ptr [[P2]], align 4
; CHECK-NEXT: br label [[IF_END1]]
; CHECK: if.end1:
; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[P1]], align 4
; CHECK-NEXT: [[COND2:%.*]] = icmp eq i32 [[B]], 0
; CHECK-NEXT: br i1 [[COND2]], label [[IF_THEN2:%.*]], label [[IF_END2:%.*]]
; CHECK: if.then2:
; CHECK-NEXT: store i32 100, ptr [[P2]], align 4
; CHECK-NEXT: br label [[IF_END2]]
; CHECK: if.end2:
; CHECK-NEXT: ret i32 0
;
entry:
store i32 42, ptr %p1
%cond1 = icmp eq i32 %a, 0
br i1 %cond1, label %if.then1, label %if.end1

if.then1:
store i32 100, ptr %p2 ; May alias with the load below
br label %if.end1

if.end1:
%val = load i32, ptr %p1 ; This load prevents merging due to potential alias
%cond2 = icmp eq i32 %b, 0
br i1 %cond2, label %if.then2, label %if.end2

if.then2:
store i32 100, ptr %p2
br label %if.end2

if.end2:
ret i32 0
}

; Test that two if-regions are not merged when there's potential aliasing
; between a store in the first if-region and a function call in the second if-region's header
define i32 @test_alias_2(i32 %a, i32 %b) {
; CHECK-LABEL: define i32 @test_alias_2
; CHECK-SAME: (i32 [[A:%.*]], i32 [[B:%.*]]) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[P:%.*]] = alloca i32, align 4
; CHECK-NEXT: store i32 42, ptr [[P]], align 4
; CHECK-NEXT: [[COND1:%.*]] = icmp eq i32 [[A]], 0
; CHECK-NEXT: br i1 [[COND1]], label [[IF_THEN1:%.*]], label [[IF_END1:%.*]]
; CHECK: if.then1:
; CHECK-NEXT: store i32 100, ptr @g, align 4
; CHECK-NEXT: br label [[IF_END1]]
; CHECK: if.end1:
; CHECK-NEXT: call void @foo()
; CHECK-NEXT: [[COND2:%.*]] = icmp eq i32 [[B]], 0
; CHECK-NEXT: br i1 [[COND2]], label [[IF_THEN2:%.*]], label [[IF_END2:%.*]]
; CHECK: if.then2:
; CHECK-NEXT: store i32 100, ptr @g, align 4
; CHECK-NEXT: br label [[IF_END2]]
; CHECK: if.end2:
; CHECK-NEXT: ret i32 0
;
entry:
%p = alloca i32
store i32 42, ptr %p
%cond1 = icmp eq i32 %a, 0
br i1 %cond1, label %if.then1, label %if.end1

if.then1:
store i32 100, ptr @g
br label %if.end1

if.end1:
call void @foo()
%cond2 = icmp eq i32 %b, 0
br i1 %cond2, label %if.then2, label %if.end2

if.then2:
store i32 100, ptr @g
br label %if.end2

if.end2:
ret i32 0
}

; Test that two if-regions are merged when there's no potential aliasing
; between a store in the first if-region and a load in the second if-region's header
define i32 @test_no_alias(i32 %a, i32 %b) {
; CHECK-LABEL: define i32 @test_no_alias
; CHECK-SAME: (i32 [[A:%.*]], i32 [[B:%.*]]) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[P:%.*]] = alloca i32, align 4
; CHECK-NEXT: store i32 42, ptr [[P]], align 4
; CHECK-NEXT: [[COND1:%.*]] = icmp eq i32 [[A]], 0
; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr @g, align 4
; CHECK-NEXT: [[COND2:%.*]] = icmp eq i32 [[B]], 0
; CHECK-NEXT: [[TMP0:%.*]] = or i1 [[COND1]], [[COND2]]
; CHECK-NEXT: br i1 [[TMP0]], label [[IF_THEN2:%.*]], label [[IF_END2:%.*]]
; CHECK: if.then2:
; CHECK-NEXT: store i32 100, ptr [[P]], align 4
; CHECK-NEXT: call void @bar()
; CHECK-NEXT: br label [[IF_END2]]
; CHECK: if.end2:
; CHECK-NEXT: ret i32 0
;
entry:
%p = alloca i32
store i32 42, ptr %p
%cond1 = icmp eq i32 %a, 0
br i1 %cond1, label %if.then1, label %if.end1

if.then1:
store i32 100, ptr %p ; No alias with the load below
call void @bar() ; No alias with the load below since it's a pure function
br label %if.end1

if.end1:
%val = load i32, ptr @g
%cond2 = icmp eq i32 %b, 0
br i1 %cond2, label %if.then2, label %if.end2

if.then2:
store i32 100, ptr %p
call void @bar()
br label %if.end2

if.end2:
ret i32 0
}

attributes #1 = { readnone willreturn nounwind }
15 changes: 15 additions & 0 deletions llvm/unittests/Analysis/AliasAnalysisTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ TEST_F(AliasAnalysisTest, getModRefInfo) {
AtomicRMWInst::Xchg, Addr, ConstantInt::get(IntType, 1), Alignment,
AtomicOrdering::Monotonic, SyncScope::System, BB);

FunctionType *FooBarTy = FunctionType::get(Type::getVoidTy(C), {}, false);
Function::Create(FooBarTy, Function::ExternalLinkage, "foo", &M);
auto *BarF = Function::Create(FooBarTy, Function::ExternalLinkage, "bar", &M);
BarF->setDoesNotAccessMemory();

const Instruction *Foo = CallInst::Create(M.getFunction("foo"), {}, BB);
const Instruction *Bar = CallInst::Create(M.getFunction("bar"), {}, BB);

ReturnInst::Create(C, nullptr, BB);

auto &AA = getAAResults(*F);
Expand All @@ -203,6 +211,13 @@ TEST_F(AliasAnalysisTest, getModRefInfo) {
EXPECT_EQ(AA.getModRefInfo(CmpXChg1, std::nullopt), ModRefInfo::ModRef);
EXPECT_EQ(AA.getModRefInfo(AtomicRMW, MemoryLocation()), ModRefInfo::ModRef);
EXPECT_EQ(AA.getModRefInfo(AtomicRMW, std::nullopt), ModRefInfo::ModRef);
EXPECT_EQ(AA.getModRefInfo(Store1, Load1), ModRefInfo::ModRef);
EXPECT_EQ(AA.getModRefInfo(Store1, Store1), ModRefInfo::ModRef);
EXPECT_EQ(AA.getModRefInfo(Store1, Add1), ModRefInfo::NoModRef);
EXPECT_EQ(AA.getModRefInfo(Store1, Foo), ModRefInfo::ModRef);
EXPECT_EQ(AA.getModRefInfo(Store1, Bar), ModRefInfo::NoModRef);
EXPECT_EQ(AA.getModRefInfo(Foo, Bar), ModRefInfo::NoModRef);
EXPECT_EQ(AA.getModRefInfo(Foo, Foo), ModRefInfo::ModRef);
}

static Instruction *getInstructionByName(Function &F, StringRef Name) {
Expand Down
Loading