-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.
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
[BasicAA] Make isNotCapturedBeforeOrAt() check for calls more precise #69931
Conversation
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesFor calls, we are only interested in captures before the call, not captures by the call itself -- arguments that get passed to the call are checked explicitly. In particular, the current implementation is not optimal if the pointer is captured via a readonly argument -- in that case, we know that even if the argument is captured, the call will not modify the argument (at least not via that argument). Make this more precise by renaming to isCapturedBefore() and adding an OrAt argument that allows us to toggle whether to consider captures in the instruction itself or not. Full diff: https://github.com/llvm/llvm-project/pull/69931.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Analysis/AliasAnalysis.h b/llvm/include/llvm/Analysis/AliasAnalysis.h
index a69c4adf28fed04..00dcc185198fe9d 100644
--- a/llvm/include/llvm/Analysis/AliasAnalysis.h
+++ b/llvm/include/llvm/Analysis/AliasAnalysis.h
@@ -152,8 +152,8 @@ raw_ostream &operator<<(raw_ostream &OS, AliasResult AR);
/// Virtual base class for providers of capture information.
struct CaptureInfo {
virtual ~CaptureInfo() = 0;
- virtual bool isNotCapturedBeforeOrAt(const Value *Object,
- const Instruction *I) = 0;
+ virtual bool isNotCapturedBefore(const Value *Object, const Instruction *I,
+ bool OrAt) = 0;
};
/// Context-free CaptureInfo provider, which computes and caches whether an
@@ -163,8 +163,8 @@ class SimpleCaptureInfo final : public CaptureInfo {
SmallDenseMap<const Value *, bool, 8> IsCapturedCache;
public:
- bool isNotCapturedBeforeOrAt(const Value *Object,
- const Instruction *I) override;
+ bool isNotCapturedBefore(const Value *Object, const Instruction *I,
+ bool OrAt) override;
};
/// Context-sensitive CaptureInfo provider, which computes and caches the
@@ -191,8 +191,8 @@ class EarliestEscapeInfo final : public CaptureInfo {
const SmallPtrSetImpl<const Value *> *EphValues = nullptr)
: DT(DT), LI(LI), EphValues(EphValues) {}
- bool isNotCapturedBeforeOrAt(const Value *Object,
- const Instruction *I) override;
+ bool isNotCapturedBefore(const Value *Object, const Instruction *I,
+ bool OrAt) override;
void removeInstruction(Instruction *I);
};
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index b51659001295716..82289292a30b16d 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -194,13 +194,23 @@ static bool isObjectSize(const Value *V, uint64_t Size, const DataLayout &DL,
CaptureInfo::~CaptureInfo() = default;
-bool SimpleCaptureInfo::isNotCapturedBeforeOrAt(const Value *Object,
- const Instruction *I) {
+bool SimpleCaptureInfo::isNotCapturedBefore(const Value *Object,
+ const Instruction *I,
+ bool OrAt) {
return isNonEscapingLocalObject(Object, &IsCapturedCache);
}
-bool EarliestEscapeInfo::isNotCapturedBeforeOrAt(const Value *Object,
- const Instruction *I) {
+static bool isNotInCycle(const Instruction *I, const DominatorTree &DT,
+ const LoopInfo *LI) {
+ BasicBlock *BB = const_cast<BasicBlock *>(I->getParent());
+ SmallVector<BasicBlock *> Succs(successors(BB));
+ return Succs.empty() ||
+ !isPotentiallyReachableFromMany(Succs, BB, nullptr, &DT, LI);
+}
+
+bool EarliestEscapeInfo::isNotCapturedBefore(const Value *Object,
+ const Instruction *I,
+ bool OrAt) {
if (!isIdentifiedFunctionLocal(Object))
return false;
@@ -220,8 +230,13 @@ bool EarliestEscapeInfo::isNotCapturedBeforeOrAt(const Value *Object,
if (!Iter.first->second)
return true;
- return I != Iter.first->second &&
- !isPotentiallyReachable(Iter.first->second, I, nullptr, &DT, LI);
+ if (I == Iter.first->second) {
+ if (OrAt)
+ return true;
+ return isNotInCycle(I, DT, LI);
+ }
+
+ return !isPotentiallyReachable(Iter.first->second, I, nullptr, &DT, LI);
}
void EarliestEscapeInfo::removeInstruction(Instruction *I) {
@@ -887,7 +902,7 @@ ModRefInfo BasicAAResult::getModRefInfo(const CallBase *Call,
// Make sure the object has not escaped here, and then check that none of the
// call arguments alias the object below.
if (!isa<Constant>(Object) && Call != Object &&
- AAQI.CI->isNotCapturedBeforeOrAt(Object, Call)) {
+ AAQI.CI->isNotCapturedBefore(Object, Call, /*OrAt*/ false)) {
// Optimistically assume that call doesn't touch Object and check this
// assumption in the following loop.
@@ -1504,10 +1519,10 @@ AliasResult BasicAAResult::aliasCheck(const Value *V1, LocationSize V1Size,
// location if that memory location doesn't escape. Or it may pass a
// nocapture value to other functions as long as they don't capture it.
if (isEscapeSource(O1) &&
- AAQI.CI->isNotCapturedBeforeOrAt(O2, cast<Instruction>(O1)))
+ AAQI.CI->isNotCapturedBefore(O2, cast<Instruction>(O1), /*OrAt*/true))
return AliasResult::NoAlias;
if (isEscapeSource(O2) &&
- AAQI.CI->isNotCapturedBeforeOrAt(O1, cast<Instruction>(O2)))
+ AAQI.CI->isNotCapturedBefore(O1, cast<Instruction>(O2), /*OrAt*/true))
return AliasResult::NoAlias;
}
@@ -1700,12 +1715,7 @@ bool BasicAAResult::isValueEqualInPotentialCycles(const Value *V,
if (!Inst || Inst->getParent()->isEntryBlock())
return true;
- // Check whether the instruction is part of a cycle, by checking whether the
- // block can (non-trivially) reach itself.
- BasicBlock *BB = const_cast<BasicBlock *>(Inst->getParent());
- SmallVector<BasicBlock *> Succs(successors(BB));
- return !Succs.empty() &&
- !isPotentiallyReachableFromMany(Succs, BB, nullptr, DT);
+ return isNotInCycle(Inst, *DT, /*LI*/nullptr);
}
/// Computes the symbolic difference between two de-composed GEPs.
diff --git a/llvm/test/Transforms/GVN/captured-before.ll b/llvm/test/Transforms/GVN/captured-before.ll
index 6bf95de9ee0b5f4..62529b8c1cf62db 100644
--- a/llvm/test/Transforms/GVN/captured-before.ll
+++ b/llvm/test/Transforms/GVN/captured-before.ll
@@ -83,8 +83,7 @@ define i32 @test_capture_readonly() {
; CHECK-NEXT: [[A:%.*]] = alloca i32, align 4
; CHECK-NEXT: store i32 123, ptr [[A]], align 4
; CHECK-NEXT: call void @capture(ptr readonly [[A]])
-; CHECK-NEXT: [[V:%.*]] = load i32, ptr [[A]], align 4
-; CHECK-NEXT: ret i32 [[V]]
+; CHECK-NEXT: ret i32 123
;
%a = alloca i32
store i32 123, ptr %a
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
9ad5d52
to
c5c8650
Compare
ping |
ping :) |
ping ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@@ -152,8 +152,8 @@ raw_ostream &operator<<(raw_ostream &OS, AliasResult AR); | |||
/// Virtual base class for providers of capture information. | |||
struct CaptureInfo { | |||
virtual ~CaptureInfo() = 0; | |||
virtual bool isNotCapturedBeforeOrAt(const Value *Object, | |||
const Instruction *I) = 0; | |||
virtual bool isNotCapturedBefore(const Value *Object, const Instruction *I, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are here it would be good to document the function
For calls, we are only interested in captures before the call, not captures by the call itself -- arguments that get passed to the call are checked explicitly. In particular, the current implementation is not optimal if the pointer is captured via a readonly argument -- in that case, we know that even if the argument is captured, the call will not modify the argument (at least not via that argument). Make this more precise by renaming to isCapturedBefore() and adding an OrAt argument that allows us to toggle whether to consider captures in the instruction itself or not.
c5c8650
to
10057c2
Compare
I'm seeing miscompilations; this seems to miss something when it comes to vectors. Consider: define i32 @f(<1 x i32> %index, i32 %val) {
entry:
%alloca = alloca i32
store i32 %val, ptr %alloca
%ptrs = getelementptr inbounds i32, ptr %alloca, <1 x i32> %index
call void @g(<1 x ptr> %ptrs)
%reload = load i32, ptr %alloca
ret i32 %reload
}
declare void @g(...) Here, it is not valid to optimise away ; ModuleID = 'test.ll'
source_filename = "test.ll"
define i32 @f(<1 x i32> %index, i32 %val) {
entry:
%alloca = alloca i32, align 4
store i32 %val, ptr %alloca, align 4
%ptrs = getelementptr inbounds i32, ptr %alloca, <1 x i32> %index
call void @g(<1 x ptr> %ptrs)
ret i32 %val
}
declare void @g(...) Is this something you can take a quick look at, or should we revert this for now? |
Thank you for the prompt response! I am surprised as well it only showed up now, but happy to report that after updating to a newer LLVM that includes your fix, I have not seen further issues. |
For calls, we are only interested in captures before the call, not captures by the call itself -- arguments that get passed to the call are checked explicitly.
In particular, the current implementation is not optimal if the pointer is captured via a readonly argument -- in that case, we know that even if the argument is captured, the call will not modify the argument (at least not via that argument).
Make this more precise by renaming to isCapturedBefore() and adding an OrAt argument that allows us to toggle whether to consider captures in the instruction itself or not.