[CaptureTracking][FunctionAttrs] Add support for CaptureInfo #125880
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
nikic
force-pushed
the
capture-info-inference
branch
from
5be3d8b to
2f698e2
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesThis extends CaptureTracking to support inferring non-trivial CaptureInfos. The focus of this patch is to only support FunctionAttrs, other users of CaptureTracking will be updated in followups. The key API changes here are:
For now, this patch retains the (unsound) special logic for comparison of null with a dereferenceable pointer. I'd like to switch key code to take advantage of address/address_is_null before dropping it. This PR mainly intends to introduce necessary API changes and basic inference support, there are various possible improvements marked with TODOs. Patch is 69.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/125880.diff 25 Files Affected:
diff --git a/clang/test/CodeGen/allow-ubsan-check.c b/clang/test/CodeGen/allow-ubsan-check.c
index 0cd81a77f5cc594..c116604288546dc 100644
--- a/clang/test/CodeGen/allow-ubsan-check.c
+++ b/clang/test/CodeGen/allow-ubsan-check.c
@@ -86,7 +86,7 @@ int div(int x, int y) {
}
// CHECK-LABEL: define dso_local i32 @null(
-// CHECK-SAME: ptr noundef readonly [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-SAME: ptr noundef readonly captures(address_is_null) [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
// CHECK-NEXT: [[ENTRY:.*:]]
// CHECK-NEXT: [[TMP0:%.*]] = icmp eq ptr [[X]], null, !nosanitize [[META2]]
//
@@ -102,7 +102,7 @@ int div(int x, int y) {
// CHECK-NEXT: ret i32 [[TMP2]]
//
// TR-LABEL: define dso_local i32 @null(
-// TR-SAME: ptr noundef readonly [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// TR-SAME: ptr noundef readonly captures(address_is_null) [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
// TR-NEXT: [[ENTRY:.*:]]
// TR-NEXT: [[TMP0:%.*]] = icmp eq ptr [[X]], null, !nosanitize [[META2]]
// TR-NEXT: [[TMP1:%.*]] = tail call i1 @llvm.allow.ubsan.check(i8 29), !nosanitize [[META2]]
@@ -116,7 +116,7 @@ int div(int x, int y) {
// TR-NEXT: ret i32 [[TMP2]]
//
// REC-LABEL: define dso_local i32 @null(
-// REC-SAME: ptr noundef readonly [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// REC-SAME: ptr noundef readonly captures(address_is_null) [[X:%.*]]) local_unnamed_addr #[[ATTR0]] {
// REC-NEXT: [[ENTRY:.*:]]
// REC-NEXT: [[TMP0:%.*]] = icmp eq ptr [[X]], null, !nosanitize [[META2]]
// REC-NEXT: [[TMP1:%.*]] = tail call i1 @llvm.allow.ubsan.check(i8 29), !nosanitize [[META2]]
diff --git a/clang/test/CodeGenCXX/RelativeVTablesABI/dynamic-cast.cpp b/clang/test/CodeGenCXX/RelativeVTablesABI/dynamic-cast.cpp
index 83daf57be22ffcc..3662a270713b697 100644
--- a/clang/test/CodeGenCXX/RelativeVTablesABI/dynamic-cast.cpp
+++ b/clang/test/CodeGenCXX/RelativeVTablesABI/dynamic-cast.cpp
@@ -3,7 +3,7 @@
// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O3 -o - -emit-llvm | FileCheck %s
-// CHECK: define{{.*}} ptr @_Z6upcastP1B(ptr noundef readnone returned %b) local_unnamed_addr
+// CHECK: define{{.*}} ptr @_Z6upcastP1B(ptr noundef readnone returned captures(ret: address, provenance) %b) local_unnamed_addr
// CHECK-NEXT: entry:
// CHECK-NEXT: ret ptr %b
// CHECK-NEXT: }
@@ -22,12 +22,12 @@
// CHECK: declare ptr @__dynamic_cast(ptr, ptr, ptr, i64) local_unnamed_addr
-// CHECK: define{{.*}} ptr @_Z8selfcastP1B(ptr noundef readnone returned %b) local_unnamed_addr
+// CHECK: define{{.*}} ptr @_Z8selfcastP1B(ptr noundef readnone returned captures(ret: address, provenance) %b) local_unnamed_addr
// CHECK-NEXT: entry
// CHECK-NEXT: ret ptr %b
// CHECK-NEXT: }
-// CHECK: define{{.*}} ptr @_Z9void_castP1B(ptr noundef readonly %b) local_unnamed_addr
+// CHECK: define{{.*}} ptr @_Z9void_castP1B(ptr noundef readonly captures(address_is_null, ret: address, provenance) %b) local_unnamed_addr
// CHECK-NEXT: entry:
// CHECK-NEXT: [[isnull:%[0-9]+]] = icmp eq ptr %b, null
// CHECK-NEXT: br i1 [[isnull]], label %[[dynamic_cast_end:[a-z0-9._]+]], label %[[dynamic_cast_notnull:[a-z0-9._]+]]
diff --git a/clang/test/CodeGenCXX/RelativeVTablesABI/type-info.cpp b/clang/test/CodeGenCXX/RelativeVTablesABI/type-info.cpp
index c471e5dbd7b33ce..2a838708ca23152 100644
--- a/clang/test/CodeGenCXX/RelativeVTablesABI/type-info.cpp
+++ b/clang/test/CodeGenCXX/RelativeVTablesABI/type-info.cpp
@@ -24,7 +24,7 @@
// CHECK-NEXT: ret ptr @_ZTS1A
// CHECK-NEXT: }
-// CHECK: define{{.*}} i1 @_Z5equalP1A(ptr noundef readonly %a) local_unnamed_addr
+// CHECK: define{{.*}} i1 @_Z5equalP1A(ptr noundef readonly captures(address_is_null) %a) local_unnamed_addr
// CHECK-NEXT: entry:
// CHECK-NEXT: [[isnull:%[0-9]+]] = icmp eq ptr %a, null
// CHECK-NEXT: br i1 [[isnull]], label %[[bad_typeid:[a-z0-9._]+]], label %[[end:[a-z0-9.+]+]]
diff --git a/clang/test/CodeGenOpenCL/amdgcn-buffer-rsrc-type.cl b/clang/test/CodeGenOpenCL/amdgcn-buffer-rsrc-type.cl
index 0aadaad2dca5c52..62fd20c4d141455 100644
--- a/clang/test/CodeGenOpenCL/amdgcn-buffer-rsrc-type.cl
+++ b/clang/test/CodeGenOpenCL/amdgcn-buffer-rsrc-type.cl
@@ -22,7 +22,7 @@ __amdgpu_buffer_rsrc_t getBuffer(void *p) {
}
// CHECK-LABEL: define {{[^@]+}}@consumeBufferPtr
-// CHECK-SAME: (ptr addrspace(5) noundef readonly [[P:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-SAME: (ptr addrspace(5) noundef readonly captures(address) [[P:%.*]]) local_unnamed_addr #[[ATTR0]] {
// CHECK-NEXT: entry:
// CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq ptr addrspace(5) [[P]], addrspacecast (ptr null to ptr addrspace(5))
// CHECK-NEXT: br i1 [[TOBOOL_NOT]], label [[IF_END:%.*]], label [[IF_THEN:%.*]]
@@ -39,7 +39,7 @@ void consumeBufferPtr(__amdgpu_buffer_rsrc_t *p) {
}
// CHECK-LABEL: define {{[^@]+}}@test
-// CHECK-SAME: (ptr addrspace(5) noundef readonly [[A:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-SAME: (ptr addrspace(5) noundef readonly captures(address) [[A:%.*]]) local_unnamed_addr #[[ATTR0]] {
// CHECK-NEXT: entry:
// CHECK-NEXT: [[TMP0:%.*]] = load i32, ptr addrspace(5) [[A]], align 16, !tbaa [[TBAA8:![0-9]+]]
// CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[TMP0]], 0
diff --git a/clang/test/CodeGenOpenCL/as_type.cl b/clang/test/CodeGenOpenCL/as_type.cl
index 1fe26fbeafdb4b8..2c6cdc3810b4da4 100644
--- a/clang/test/CodeGenOpenCL/as_type.cl
+++ b/clang/test/CodeGenOpenCL/as_type.cl
@@ -67,7 +67,7 @@ int3 f8(char16 x) {
return __builtin_astype(x, int3);
}
-//CHECK: define{{.*}} spir_func noundef ptr addrspace(1) @addr_cast(ptr noundef readnone %[[x:.*]])
+//CHECK: define{{.*}} spir_func noundef ptr addrspace(1) @addr_cast(ptr noundef readnone captures(ret: address, provenance) %[[x:.*]])
//CHECK: %[[cast:.*]] ={{.*}} addrspacecast ptr %[[x]] to ptr addrspace(1)
//CHECK: ret ptr addrspace(1) %[[cast]]
global int* addr_cast(int *x) {
diff --git a/llvm/include/llvm/Analysis/CaptureTracking.h b/llvm/include/llvm/Analysis/CaptureTracking.h
index 06a00d9ae789908..4e09c1e6f3021dc 100644
--- a/llvm/include/llvm/Analysis/CaptureTracking.h
+++ b/llvm/include/llvm/Analysis/CaptureTracking.h
@@ -14,11 +14,13 @@
#define LLVM_ANALYSIS_CAPTURETRACKING_H
#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/ModRef.h"
namespace llvm {
class Value;
class Use;
+ class CaptureInfo;
class DataLayout;
class Instruction;
class DominatorTree;
@@ -94,10 +96,38 @@ namespace llvm {
/// U->getUser() is always an Instruction.
virtual bool shouldExplore(const Use *U);
- /// captured - Information about the pointer was captured by the user of
- /// use U. Return true to stop the traversal or false to continue looking
- /// for more capturing instructions.
- virtual bool captured(const Use *U) = 0;
+ /// When returned from captures(), stop the traversal.
+ static std::optional<CaptureComponents> stop() { return std::nullopt; }
+
+ /// When returned from captures(), continue traversal, but do not follow
+ /// the return value of this user, even if it has additional capture
+ /// components. Should only be used if captures() has already taken the
+ /// potential return caputres into account.
+ static std::optional<CaptureComponents> continueIgnoringReturn() {
+ return CaptureComponents::None;
+ }
+
+ /// When returned from captures(), continue traversal, and also follow
+ /// the return value of this user if it has additional capture components
+ /// (that is, capture components in Ret that are not part of Other).
+ static std::optional<CaptureComponents> continueDefault(CaptureInfo CI) {
+ CaptureComponents RetCC = CI.getRetComponents();
+ if (!capturesNothing(RetCC & ~CI.getOtherComponents()))
+ return RetCC;
+ return CaptureComponents::None;
+ }
+
+ /// Use U directly captures CI.getOtherComponents() and additionally
+ /// CI.getRetComponents() through the return value of the user of U.
+ ///
+ /// Return std::nullopt to stop the traversal, or the CaptureComponents to
+ /// follow via the return value, which must be a subset of
+ /// CI.getRetComponents().
+ ///
+ /// For convenience, prefer returning one of stop(), continueDefault(CI) or
+ /// continueIgnoringReturn().
+ virtual std::optional<CaptureComponents> captured(const Use *U,
+ CaptureInfo CI) = 0;
/// isDereferenceableOrNull - Overload to allow clients with additional
/// knowledge about pointer dereferenceability to provide it and thereby
@@ -105,20 +135,14 @@ namespace llvm {
virtual bool isDereferenceableOrNull(Value *O, const DataLayout &DL);
};
- /// Types of use capture kinds, see \p DetermineUseCaptureKind.
- enum class UseCaptureKind {
- NO_CAPTURE,
- MAY_CAPTURE,
- PASSTHROUGH,
- };
-
/// Determine what kind of capture behaviour \p U may exhibit.
///
- /// A use can be no-capture, a use can potentially capture, or a use can be
- /// passthrough such that the uses of the user or \p U should be inspected.
+ /// The Other part of the returned CaptureInfo indicates which component of
+ /// the pointer may be captured directly by the use. The Ret part indicates
+ /// which components may be captured by following uses of the user of \p U.
/// The \p IsDereferenceableOrNull callback is used to rule out capturing for
/// certain comparisons.
- UseCaptureKind
+ CaptureInfo
DetermineUseCaptureKind(const Use &U,
llvm::function_ref<bool(Value *, const DataLayout &)>
IsDereferenceableOrNull);
diff --git a/llvm/include/llvm/Support/ModRef.h b/llvm/include/llvm/Support/ModRef.h
index a8ce9a8e6e69c47..716ed2cb8cd4871 100644
--- a/llvm/include/llvm/Support/ModRef.h
+++ b/llvm/include/llvm/Support/ModRef.h
@@ -309,6 +309,10 @@ inline bool capturesFullProvenance(CaptureComponents CC) {
return (CC & CaptureComponents::Provenance) == CaptureComponents::Provenance;
}
+inline bool capturesAll(CaptureComponents CC) {
+ return CC == CaptureComponents::All;
+}
+
raw_ostream &operator<<(raw_ostream &OS, CaptureComponents CC);
/// Represents which components of the pointer may be captured in which
@@ -333,6 +337,22 @@ class CaptureInfo {
/// Create CaptureInfo that may capture all components of the pointer.
static CaptureInfo all() { return CaptureInfo(CaptureComponents::All); }
+ /// Create CaptureInfo that may only capture through means other than the
+ /// return value.
+ static CaptureInfo
+ otherOnly(CaptureComponents OtherComponents = CaptureComponents::All) {
+ return CaptureInfo(OtherComponents, CaptureComponents::None);
+ }
+
+ /// Create CaptureInfo that may only capture via the return value.
+ static CaptureInfo
+ retOnly(CaptureComponents RetComponents = CaptureComponents::All) {
+ return CaptureInfo(CaptureComponents::None, RetComponents);
+ }
+
+ /// Whether the pointer is only captured via the return value.
+ bool isRetOnly() const { return capturesNothing(OtherComponents); }
+
/// Get components potentially captured by the return value.
CaptureComponents getRetComponents() const { return RetComponents; }
diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp
index 49baf2eb84bb3e1..4e403b8825c7f71 100644
--- a/llvm/lib/Analysis/CaptureTracking.cpp
+++ b/llvm/lib/Analysis/CaptureTracking.cpp
@@ -81,14 +81,16 @@ struct SimpleCaptureTracker : public CaptureTracker {
Captured = true;
}
- bool captured(const Use *U) override {
+ std::optional<CaptureComponents> captured(const Use *U,
+ CaptureInfo CI) override {
+ // TODO(captures): Use CaptureInfo.
if (isa<ReturnInst>(U->getUser()) && !ReturnCaptures)
- return false;
+ return continueIgnoringReturn();
LLVM_DEBUG(dbgs() << "Captured by: " << *U->getUser() << "\n");
Captured = true;
- return true;
+ return stop();
}
bool ReturnCaptures;
@@ -122,19 +124,22 @@ struct CapturesBefore : public CaptureTracker {
return !isPotentiallyReachable(I, BeforeHere, nullptr, DT, LI);
}
- bool captured(const Use *U) override {
+ std::optional<CaptureComponents> captured(const Use *U,
+ CaptureInfo CI) override {
+ // TODO(captures): Use CaptureInfo.
Instruction *I = cast<Instruction>(U->getUser());
if (isa<ReturnInst>(I) && !ReturnCaptures)
- return false;
+ return continueIgnoringReturn();
// Check isSafeToPrune() here rather than in shouldExplore() to avoid
// an expensive reachability query for every instruction we look at.
// Instead we only do one for actual capturing candidates.
if (isSafeToPrune(I))
- return false;
+ // If the use is not reachable, the instruction result isn't either.
+ return continueIgnoringReturn();
Captured = true;
- return true;
+ return stop();
}
const Instruction *BeforeHere;
@@ -166,10 +171,12 @@ struct EarliestCaptures : public CaptureTracker {
EarliestCapture = &*F.getEntryBlock().begin();
}
- bool captured(const Use *U) override {
+ std::optional<CaptureComponents> captured(const Use *U,
+ CaptureInfo CI) override {
+ // TODO(captures): Use CaptureInfo.
Instruction *I = cast<Instruction>(U->getUser());
if (isa<ReturnInst>(I) && !ReturnCaptures)
- return false;
+ return continueIgnoringReturn();
if (!EarliestCapture)
EarliestCapture = I;
@@ -177,9 +184,10 @@ struct EarliestCaptures : public CaptureTracker {
EarliestCapture = DT.findNearestCommonDominator(EarliestCapture, I);
Captured = true;
- // Return false to continue analysis; we need to see all potential
- // captures.
- return false;
+ // Continue analysis, as we need to see all potential captures. However,
+ // we do not need to follow the instruction result, as this use will
+ // dominate any captures made through the instruction result..
+ return continueIgnoringReturn();
}
Instruction *EarliestCapture = nullptr;
@@ -274,25 +282,26 @@ Instruction *llvm::FindEarliestCapture(const Value *V, Function &F,
return CB.EarliestCapture;
}
-UseCaptureKind llvm::DetermineUseCaptureKind(
+CaptureInfo llvm::DetermineUseCaptureKind(
const Use &U,
function_ref<bool(Value *, const DataLayout &)> IsDereferenceableOrNull) {
Instruction *I = dyn_cast<Instruction>(U.getUser());
// TODO: Investigate non-instruction uses.
if (!I)
- return UseCaptureKind::MAY_CAPTURE;
+ return CaptureInfo::otherOnly();
switch (I->getOpcode()) {
case Instruction::Call:
case Instruction::Invoke: {
+ // TODO(captures): Make this more precise.
auto *Call = cast<CallBase>(I);
// Not captured if the callee is readonly, doesn't return a copy through
// its return value and doesn't unwind (a readonly function can leak bits
// by throwing an exception or not depending on the input value).
if (Call->onlyReadsMemory() && Call->doesNotThrow() &&
Call->getType()->isVoidTy())
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::none();
// The pointer is not captured if returned pointer is not captured.
// NOTE: CaptureTracking users should not assume that only functions
@@ -300,13 +309,13 @@ UseCaptureKind llvm::DetermineUseCaptureKind(
// getUnderlyingObject in ValueTracking or DecomposeGEPExpression
// in BasicAA also need to know about this property.
if (isIntrinsicReturningPointerAliasingArgumentWithoutCapturing(Call, true))
- return UseCaptureKind::PASSTHROUGH;
+ return CaptureInfo::retOnly();
// Volatile operations effectively capture the memory location that they
// load and store to.
if (auto *MI = dyn_cast<MemIntrinsic>(Call))
if (MI->isVolatile())
- return UseCaptureKind::MAY_CAPTURE;
+ return CaptureInfo::otherOnly();
// Calling a function pointer does not in itself cause the pointer to
// be captured. This is a subtle point considering that (for example)
@@ -315,30 +324,26 @@ UseCaptureKind llvm::DetermineUseCaptureKind(
// captured, even though the loaded value might be the pointer itself
// (think of self-referential objects).
if (Call->isCallee(&U))
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::none();
// Not captured if only passed via 'nocapture' arguments.
assert(Call->isDataOperand(&U) && "Non-callee must be data operand");
- if (!Call->doesNotCapture(Call->getDataOperandNo(&U))) {
- // The parameter is not marked 'nocapture' - captured.
- return UseCaptureKind::MAY_CAPTURE;
- }
- return UseCaptureKind::NO_CAPTURE;
+ return Call->getCaptureInfo(Call->getDataOperandNo(&U));
}
case Instruction::Load:
// Volatile loads make the address observable.
if (cast<LoadInst>(I)->isVolatile())
- return UseCaptureKind::MAY_CAPTURE;
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::otherOnly();
+ return CaptureInfo::none();
case Instruction::VAArg:
// "va-arg" from a pointer does not cause it to be captured.
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::none();
case Instruction::Store:
// Stored the pointer - conservatively assume it may be captured.
// Volatile stores make the address observable.
if (U.getOperandNo() == 0 || cast<StoreInst>(I)->isVolatile())
- return UseCaptureKind::MAY_CAPTURE;
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::otherOnly();
+ return CaptureInfo::none();
case Instruction::AtomicRMW: {
// atomicrmw conceptually includes both a load and store from
// the same location.
@@ -347,8 +352,8 @@ UseCaptureKind llvm::DetermineUseCaptureKind(
// Volatile stores make the address observable.
auto *ARMWI = cast<AtomicRMWInst>(I);
if (U.getOperandNo() == 1 || ARMWI->isVolatile())
- return UseCaptureKind::MAY_CAPTURE;
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::otherOnly();
+ return CaptureInfo::none();
}
case Instruction::AtomicCmpXchg: {
// cmpxchg conceptually includes both a load and store from
@@ -358,31 +363,34 @@ UseCaptureKind llvm::DetermineUseCaptureKind(
// Volatile stores make the address observable.
auto *ACXI = cast<AtomicCmpXchgInst>(I);
if (U.getOperandNo() == 1 || U.getOperandNo() == 2 || ACXI->isVolatile())
- return UseCaptureKind::MAY_CAPTURE;
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::otherOnly();
+ return CaptureInfo::none();
}
case Instruction::GetElementPtr:
// AA does not support pointers of vectors, so GEP vector splats need to
// be considered as captures.
if (I->getType()->isVectorTy())
- return UseCaptureKind::MAY_CAPTURE;
- return UseCaptureKind::PASSTHROUGH;
+ return CaptureInfo::otherOnly();
+ return CaptureInfo::retOnly();
case Instruction::BitCast:
case Instruction::PHI:
case Instruction::Select:
case Instruction::AddrSpaceCast:
// The original value is not captured via this if the new value isn't.
- return UseCaptureKind::PASSTHROUGH;
+ return CaptureInfo::retOnly();
case Instruction::ICmp: {
unsigned Idx = U.getOperandNo();
unsigned OtherIdx = 1 - Idx;
if (auto *CPN = dyn_cast<ConstantPointerNull>(I->getOperand(OtherIdx))) {
+ // TODO(captures): Remove these special cases once we make use of
+ // captures(address_is_null).
+
// Don't count comparisons of a no-alias return value against null as
// captures. This allows us to ignore comparisons of malloc results
// with null, for example.
if (CPN->getType()->getAddressSpace() == 0)
if (isNoAliasCall(U.get()->stripPointerCasts()))
- return UseCaptureKind::NO_CAPTURE;
+ return CaptureInfo::none();
if (!I->getFunction()->nullPointerIsDefined()) {
auto *O = I->getOperand(Idx)->stripPointerCastsSameRepresentation();
// Comparing a dereferenceable_or_null pointer against null cannot
@@ -390,17 +398,19 @@ UseCaptureKind llvm::DetermineUseCaptureKind(
// ...
[truncated]
|
nikic
requested review from
fhahn,
arichardson,
dtcxzyw,
goldsteinn and
efriedma-quic
goldsteinn
reviewed
Feb 6, 2025
goldsteinn
reviewed
Feb 6, 2025
| /// (that is, capture components in Ret that are not part of Other). | ||
| static std::optional<CaptureComponents> continueDefault(CaptureInfo CI) { | ||
| CaptureComponents RetCC = CI.getRetComponents(); | ||
| if (!capturesNothing(RetCC & ~CI.getOtherComponents())) |
There was a problem hiding this comment.
Maybe capturesSomething(...)?
There was a problem hiding this comment.
I've switched most places using !capturesNothing to capturesAnything now.
goldsteinn
reviewed
Feb 6, 2025
| } | ||
|
|
||
| /// Whether the pointer is only captured via the return value. | ||
| bool isRetOnly() const { return capturesNothing(OtherComponents); } |
There was a problem hiding this comment.
Based on usage might be more useful if this is captureThroughRetOnly and make it return capturesNothing(OtherComponent) && !capturesNothing(RetComponents).
goldsteinn
reviewed
Feb 6, 2025
|
|
||
| LLVM_DEBUG(dbgs() << "Captured by: " << *U->getUser() << "\n"); | ||
|
|
||
| Captured = true; | ||
| return true; | ||
| return stop(); |
There was a problem hiding this comment.
If Same err in my thinking as above.!ReturnCaptures, can this return other: all, ret:none?
goldsteinn
reviewed
Feb 6, 2025
| if (isa<ReturnInst>(U->getUser()) && !ReturnCaptures) | ||
| return false; | ||
| return continueIgnoringReturn(); |
There was a problem hiding this comment.
Why does Okay, this is just a CC, not a CI.continueIgnoringReturn also set other:none?
goldsteinn
reviewed
Feb 6, 2025
| case Instruction::BitCast: | ||
| case Instruction::PHI: | ||
| case Instruction::Select: | ||
| case Instruction::AddrSpaceCast: | ||
| // The original value is not captured via this if the new value isn't. | ||
| return UseCaptureKind::PASSTHROUGH; | ||
| return CaptureInfo::retOnly(); | ||
| case Instruction::ICmp: { | ||
| unsigned Idx = U.getOperandNo(); | ||
| unsigned OtherIdx = 1 - Idx; | ||
| if (auto *CPN = dyn_cast<ConstantPointerNull>(I->getOperand(OtherIdx))) { |
There was a problem hiding this comment.
Unrelated to your PR, but isn't this somewhat predicate dependent. I.e icmp slt ptr %X, null also exposes the signbit.
There was a problem hiding this comment.
That's a good point. This code is also subtly incorrect for other reasons, but the signed predicate case is blatantly wrong. Not going to touch this now as this be removed altogether soon...
There was a problem hiding this comment.
Want me to throw up a quick PR to check that pred is equality? Or leave it all for later?
There was a problem hiding this comment.
I'd prefer to leave it for later.
There was a problem hiding this comment.
I ended up including the predicate check after all, as it's also needed for the new code, not just the old one.
goldsteinn
reviewed
Feb 6, 2025
| case UseCaptureKind::MAY_CAPTURE: | ||
| if (Tracker->captured(U)) | ||
| CaptureComponents RetCC = CI.getRetComponents(); | ||
| if (!capturesNothing(CI.getOtherComponents())) { |
There was a problem hiding this comment.
Why is this not capturesNothing(CI.getOtherComponents())? As I understand this for the previous PASSTHROUGH case which should be other:none, ret:<not none>.
There was a problem hiding this comment.
This is the MAY_CAPTURE case. The PASSTHROUGH case is the one below (AddUses).
There was a problem hiding this comment.
This case falls through to the the PASSTHROUGH logic though. Or is it made equivilent because captured(...) only returns nullopt or none?
There was a problem hiding this comment.
It falls through in the case where we handle something like captures(address_is_null, ret: address, provenance). Depending on the captured() return value, we'll fall through to handling the ret part as passthrough.
There was a problem hiding this comment.
I think this code should be clearer now that it uses the enum return value.
goldsteinn
reviewed
Feb 6, 2025
goldsteinn
reviewed
Feb 6, 2025
| if (R != Attribute::None) | ||
| addAccessAttr(A, R); | ||
| // Infer the access attributes given the new captures one | ||
| DetermineAccessAttrsForSingleton(A); |
There was a problem hiding this comment.
Don't need to set changed?
There was a problem hiding this comment.
Done. It was not necessary previously because we always added nocapture. But now that it's conditional we should handle Changed separately for the access attrs.
goldsteinn
reviewed
Feb 6, 2025
| for (ArgumentGraphNode *N : ArgumentSCC) { | ||
| if (DetermineAccessAttrsForSingleton(N->Definition)) | ||
| Changed.insert(N->Definition->getParent()); | ||
| } |
There was a problem hiding this comment.
For the SCC case, we skip inferring the access attributes initially, so we have to do it after we're done handling the SCC. If we infer captures(none) we can do the access attribute inference over the whole SCC, which is what the code below this does. If we don't, we can fall back on inferring for each argument individually, which is what the new code here does.
This is more useful to do now than previously because we can have states between captures(none) and captures(all).
nikic
commented
Feb 6, 2025
| /// For convenience, prefer returning one of stop(), continueDefault(CI) or | ||
| /// continueIgnoringReturn(). | ||
| virtual std::optional<CaptureComponents> captured(const Use *U, | ||
| CaptureInfo CI) = 0; |
There was a problem hiding this comment.
Looking at this again, I think I made this API more complicated than it needs to be. I think the return value can be replaced with an enum { Stop, Continue, ContinueIgnoringReturn }. It's not really necessary to return the CaptureComponents, the calling code knows the right ones to use.
There was a problem hiding this comment.
Done. I think it's nicer now.
goldsteinn
reviewed
Feb 7, 2025
| continue; | ||
| case CaptureTracker::Continue: | ||
| // Fall through to passthrough handling, but only if RetCC contains | ||
| // additional components that OtherCC does not. |
There was a problem hiding this comment.
Whats the reasoning behind this? I think of return/other capture cases as orthogonal?
There was a problem hiding this comment.
This is to avoid wasting time by following values that cannot contribute any new capture information. For call captures, we'll usually have OtherCC == RetCC, and there's no point in following the return value in that case, as it won't contribute anything new. That would be a regression relative to current CaptureTracking behavior (which doesn't follow call return values).
There was a problem hiding this comment.
The argument being that if captures via some component by other, there is not use in refining ret as we will be hamstrung by the other condition during analysis?
There was a problem hiding this comment.
Can you leave a comment expressing as much? Might be a case where the incremental analysis improvement becomes useful.
There was a problem hiding this comment.
I expanded the comment a bit, is this what you had in mind?
goldsteinn
reviewed
Feb 7, 2025
| @@ -774,7 +774,7 @@ define i1 @captureICmpRev(ptr %x) { | |||
| define i1 @nocaptureInboundsGEPICmp(ptr %x) { | |||
| ; FNATTRS: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none) | |||
| ; FNATTRS-LABEL: define i1 @nocaptureInboundsGEPICmp | |||
| ; FNATTRS-SAME: (ptr readnone [[X:%.*]]) #[[ATTR0]] { | |||
| ; FNATTRS-SAME: (ptr readnone captures(address_is_null) [[X:%.*]]) #[[ATTR0]] { | |||
There was a problem hiding this comment.
Is this correct? I get that the cmp is somewhat ill-formed (false), but I don't see how this ever reduces to an %x == null check with a non-zero offset.
Also seems to also apply address_is_null w.o inbounds:
define i1 @nocaptureInboundsGEPICmp(ptr %x, i64 %y) {
%1 = getelementptr i8, ptr %x, i64 %y
%2 = icmp eq ptr %1, null
ret i1 %2
}
=>
define i1 @nocaptureInboundsGEPICmp(ptr readnone captures(address_is_null) %x, i64 %y) {
%1 = getelementptr i8, ptr %x, i64 %y
%2 = icmp eq ptr %1, null
ret i1 %2
}
How is that correct?
There was a problem hiding this comment.
Good catch, this is indeed incorrect.
There was a problem hiding this comment.
I've pushed a fix for this. I'm not passing the Base object to DetermineUseCaptureKind. We can only infer address_is_null if we're comparison the base to null.
It is okay to look through inbounds GEPs though, because they preserve nullness. Though InstCombine should optimize away such GEPs anyway, so maybe it's not necessary to explicitly handle here...
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
nikic
added a commit
that referenced
this pull request
Feb 27, 2025
…125880) (#128020) Relative to the previous attempt this includes two fixes: * Adjust callCapturesBefore() to not skip captures(ret: address, provenance) arguments, as these will not count as a capture at the call-site. * When visiting uses during stack slot optimization, don't skip the ModRef check for passthru captures. Calls can both modref and be passthru for captures. ------ This extends CaptureTracking to support inferring non-trivial CaptureInfos. The focus of this patch is to only support FunctionAttrs, other users of CaptureTracking will be updated in followups. The key API changes here are: * DetermineUseCaptureKind() now returns a UseCaptureInfo where the UseCC component specifies what is captured at that Use and the ResultCC component specifies what may be captured via the return value of the User. Usually only one or the other will be used (corresponding to previous MAY_CAPTURE or PASSTHROUGH results), but both may be set for call captures. * The CaptureTracking::captures() extension point is passed this UseCaptureInfo as well and then can decide what to do with it by returning an Action, which is one of: Stop: stop traversal. ContinueIgnoringReturn: continue traversal but don't follow the instruction return value. Continue: continue traversal and follow the instruction return value if it has additional CaptureComponents. For now, this patch retains the (unsound) special logic for comparison of null with a dereferenceable pointer. I'd like to switch key code to take advantage of address/address_is_null before dropping it. This PR mainly intends to introduce necessary API changes and basic inference support, there are various possible improvements marked with TODOs.
delcypher
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 28, 2025
This reverts commit 07f057b. In upstream this landed ``` commit e56a6a2 error: cannot run gpg: No such file or directory Author: Nikita Popov <[email protected]> Date: Thu Feb 27 09:38:29 2025 +0100 Reapply [CaptureTracking][FunctionAttrs] Add support for CaptureInfo (llvm#125880) (llvm#128020) ``` which is another attempt at landing llvm#125880. The `-fbounds-safety` tests were already adjusted when this was landed before but we had to revert these changes when upstream reverted $125880. So now this PR reverts the revert of the test changes. Hopefully the upstream PR doesn't get reverted again. rdar://145796299
delcypher
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 28, 2025
This reverts commit 07f057b. In upstream this landed ``` commit e56a6a2 error: cannot run gpg: No such file or directory Author: Nikita Popov <[email protected]> Date: Thu Feb 27 09:38:29 2025 +0100 Reapply [CaptureTracking][FunctionAttrs] Add support for CaptureInfo (llvm#125880) (llvm#128020) ``` which is another attempt at landing llvm#125880. The `-fbounds-safety` tests were already adjusted when this was landed before but we had to revert these changes when upstream reverted $125880. So now this PR reverts the revert of the test changes. Hopefully the upstream PR doesn't get reverted again. rdar://145796299
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Mar 3, 2025
…lvm#125880) (llvm#128020) Relative to the previous attempt this includes two fixes: * Adjust callCapturesBefore() to not skip captures(ret: address, provenance) arguments, as these will not count as a capture at the call-site. * When visiting uses during stack slot optimization, don't skip the ModRef check for passthru captures. Calls can both modref and be passthru for captures. ------ This extends CaptureTracking to support inferring non-trivial CaptureInfos. The focus of this patch is to only support FunctionAttrs, other users of CaptureTracking will be updated in followups. The key API changes here are: * DetermineUseCaptureKind() now returns a UseCaptureInfo where the UseCC component specifies what is captured at that Use and the ResultCC component specifies what may be captured via the return value of the User. Usually only one or the other will be used (corresponding to previous MAY_CAPTURE or PASSTHROUGH results), but both may be set for call captures. * The CaptureTracking::captures() extension point is passed this UseCaptureInfo as well and then can decide what to do with it by returning an Action, which is one of: Stop: stop traversal. ContinueIgnoringReturn: continue traversal but don't follow the instruction return value. Continue: continue traversal and follow the instruction return value if it has additional CaptureComponents. For now, this patch retains the (unsound) special logic for comparison of null with a dereferenceable pointer. I'd like to switch key code to take advantage of address/address_is_null before dropping it. This PR mainly intends to introduce necessary API changes and basic inference support, there are various possible improvements marked with TODOs.
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.
This extends CaptureTracking to support inferring non-trivial CaptureInfos. The focus of this patch is to only support FunctionAttrs, other users of CaptureTracking will be updated in followups.
The key API changes here are:
For now, this patch retains the (unsound) special logic for comparison of null with a dereferenceable pointer. I'd like to switch key code to take advantage of address/address_is_null before dropping it.
This PR mainly intends to introduce necessary API changes and basic inference support, there are various possible improvements marked with TODOs.