[FlattenCFG] Fix an Imprecise Usage of AA #128117
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Chengjun (Chengjunp) ChangesIn current In this patch, we take the Full diff: https://github.com/llvm/llvm-project/pull/128117.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/FlattenCFG.cpp b/llvm/lib/Transforms/Utils/FlattenCFG.cpp
index 16b4bb1981d8b..151ca06f3498a 100644
--- a/llvm/lib/Transforms/Utils/FlattenCFG.cpp
+++ b/llvm/lib/Transforms/Utils/FlattenCFG.cpp
@@ -357,8 +357,12 @@ bool FlattenCFGOpt::CompareIfRegionBlock(BasicBlock *Block1, BasicBlock *Block2,
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))
- return false;
+ if (AA) {
+ MemoryLocation Loc1 = MemoryLocation::get(&*iter1);
+ MemoryLocation Loc2 = MemoryLocation::get(&*BI);
+ if (!AA->isNoAlias(Loc1, Loc2))
+ return false;
+ }
}
}
}
diff --git a/llvm/test/Transforms/Util/flatten-cfg.ll b/llvm/test/Transforms/Util/flatten-cfg.ll
index 038dcaa47419a..bb592a7031031 100644
--- a/llvm/test/Transforms/Util/flatten-cfg.ll
+++ b/llvm/test/Transforms/Util/flatten-cfg.ll
@@ -1,5 +1,5 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
-; RUN: opt -passes=flatten-cfg -S < %s | FileCheck %s
+; RUN: opt -passes='require<aa>,flatten-cfg' -S < %s | FileCheck %s
; This test checks whether the pass completes without a crash.
@@ -309,3 +309,89 @@ if.then.y:
exit:
ret i1 %cmp.y
}
+
+; 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 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: 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
+ 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
+ br label %if.end2
+
+if.end2:
+ ret i32 0
+}
\ No newline at end of file
|
nikic
reviewed
Feb 21, 2025
| @@ -357,8 +357,12 @@ bool FlattenCFGOpt::CompareIfRegionBlock(BasicBlock *Block1, BasicBlock *Block2, | |||
| 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)) | |||
| return false; | |||
| if (AA) { | |||
There was a problem hiding this comment.
You need to return false if (!AA).
| return false; | ||
| if (AA) { | ||
| MemoryLocation Loc1 = MemoryLocation::get(&*iter1); | ||
| MemoryLocation Loc2 = MemoryLocation::get(&*BI); |
There was a problem hiding this comment.
Can you add a test where BI is a call? I expect that to crash.
There was a problem hiding this comment.
I have just added a case including this. Yeah, it will crash with my previous implementation. It should work now with my new implementation.
| if (AA) { | ||
| MemoryLocation Loc1 = MemoryLocation::get(&*iter1); | ||
| MemoryLocation Loc2 = MemoryLocation::get(&*BI); | ||
| if (!AA->isNoAlias(Loc1, Loc2)) |
There was a problem hiding this comment.
This isn't wrong (for non-calls), but getModRefInfo would be more appropriate here.
There was a problem hiding this comment.
Thank you for pointing this out. Instead of making modifications here, I choose to implement a new alias helper function that takes in two instructions. Could you please help me check whether that is the right way to use getModRefInfo check the alias between two instructions? Thanks!
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
nikic
reviewed
Feb 25, 2025
| @@ -381,6 +381,14 @@ class AAResults { | |||
| MemoryLocation::getBeforeOrAfter(V2)); | |||
| } | |||
|
|
|||
| /// A convenience wrapper around the \c isNoAlias for two instructions. | |||
| bool isNoAlias(const Instruction *I1, const Instruction *I2) { | |||
There was a problem hiding this comment.
Using NoAlias terminology isn't right. Aliasing is a concept of memory locations. For instructions, you are interested in memory effects, as provided by getModRefInfo(). Adding a getModRefInfo() overload that accepts two instructions would be fine.
There was a problem hiding this comment.
I'd expect the implementation to look somewhat similar to getModRefInfo() for an instruction and call:
llvm-project/llvm/lib/Analysis/AliasAnalysis.cpp
Lines 190 to 209 in d254fa8
|
Hi @nikic , could you help to review the change please? Thanks! |
|
Ping @nikic |
|
Ping @nikic. |
nikic
reviewed
Apr 14, 2025
| // Check whether two instructions may read or write the same memory location. | ||
| ModRefInfo AAResults::getModRefInfo(const Instruction *I1, | ||
| const Instruction *I2) { | ||
| SimpleAAQueryInfo AAQIP(*this); |
There was a problem hiding this comment.
Should accept AAQueryInfo as parameter.
There was a problem hiding this comment.
To clarify this bit: We should have a method taking AAQI, and another helper that uses SimplieAAQueryInfo and call the main method.
There was a problem hiding this comment.
I have updated the methods. Let me know whether it is you expected.
| return getModRefInfo(I1, Call2, AAQIP); | ||
|
|
||
| ModRefInfo MR = getModRefInfo(I1, MemoryLocation::getOrNone(I2), AAQIP); | ||
| return isModOrRefSet(MR) ? ModRefInfo::ModRef : ModRefInfo::NoModRef; |
There was a problem hiding this comment.
This could be more precise, but it's okay to start with.
There was a problem hiding this comment.
What would be a better / more precise way to check the ModRefInfo? Thanks!
There was a problem hiding this comment.
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:
I think it's ok if you add a FIXME.
nikic
approved these changes
Apr 17, 2025
| return getModRefInfo(I1, Call2, AAQIP); | ||
|
|
||
| ModRefInfo MR = getModRefInfo(I1, MemoryLocation::getOrNone(I2), AAQIP); | ||
| return isModOrRefSet(MR) ? ModRefInfo::ModRef : ModRefInfo::NoModRef; |
There was a problem hiding this comment.
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:
I think it's ok if you add a FIXME.
…p/llvm-project into chengjunp/FlattenCFG_AA_use_fix
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/6329 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
In current `FlattenCFG`, using `isNoAlias` for two instructions is imprecise. For example, when passing a store instruction and a load instruction directly into `AA->isNoAlias`, it will always return `NoAlias`. This happens because when checking the types of the two Values, the store instruction (which has a `void` type) causes the analysis to return `NoAlias`. For instructions, we should use `getModRefInfo` instead of `isNoAlias`, as aliasing is a concept of memory locations. In this patch, `AAResults::getModRefInfo` is supported to take in two instructions. It will check whether two instructions may access the same memory location or not. And in `FlattenCFG`, we use this new helper function to do the check instead of `isNoAlias`. Unit tests and lit tests are also included to this patch.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
In current `FlattenCFG`, using `isNoAlias` for two instructions is imprecise. For example, when passing a store instruction and a load instruction directly into `AA->isNoAlias`, it will always return `NoAlias`. This happens because when checking the types of the two Values, the store instruction (which has a `void` type) causes the analysis to return `NoAlias`. For instructions, we should use `getModRefInfo` instead of `isNoAlias`, as aliasing is a concept of memory locations. In this patch, `AAResults::getModRefInfo` is supported to take in two instructions. It will check whether two instructions may access the same memory location or not. And in `FlattenCFG`, we use this new helper function to do the check instead of `isNoAlias`. Unit tests and lit tests are also included to this patch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In current
FlattenCFG, usingisNoAliasfor two instructions is imprecise. For example, when passing a store instruction and a load instruction directly intoAA->isNoAlias, it will always returnNoAlias. This happens because when checking the types of the two Values, the store instruction (which has avoidtype) causes the analysis to returnNoAlias.For instructions, we should use
getModRefInfoinstead ofisNoAlias, as aliasing is a concept of memory locations.In this patch,
AAResults::getModRefInfois supported to take in two instructions. It will check whether two instructions may access the same memory location or not. And inFlattenCFG, we use this new helper function to do the check instead ofisNoAlias.Unit tests and lit tests are also included to this patch.