Skip to content

[CaptureTracking][NFC] Clarify usage expectations in PointerMayBeCaptured comments #132744

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

Merged
merged 7 commits into from
Apr 14, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 8 additions & 0 deletions llvm/include/llvm/Analysis/CaptureTracking.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,17 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
/// This assumes the pointer is to a function-local object. The caller is
/// responsible for ensuring this.
Copy link
Contributor

Choose a reason for hiding this comment

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

This depends on the use case. In most cases you'd want to pass a function-local object, but e.g. to infer captures() attribute you'd just work with arbitrary function arguments.

I think I'd add something like this instead:

This function only considers captures of the passed value via its use-def chain, without considering captures of values it may be based on, or implicit captures such as for external globals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This depends on the use case. In most cases you'd want to pass a function-local object, but e.g. to infer captures() attribute you'd just work with arbitrary function arguments.

I think I'd add something like this instead:

This function only considers captures of the passed value via its use-def chain, without considering captures of values it may be based on, or implicit captures such as for external globals.

Good point, and thank you for the improved phrasing! Your suggested comment is indeed more appropriate. Just a minor correction: it should be 'def-use chain' rather than 'use-def chain' because our CaptureAnalysis works by forward tracking using getUser.

bool PointerMayBeCaptured(const Value *V, bool ReturnCaptures,
unsigned MaxUsesToExplore = 0);

/// Return which components of the pointer may be captured. Only consider
/// components that are part of \p Mask. Once \p StopFn on the accumulated
/// components returns true, the traversal is aborted early. By default, this
/// happens when *any* of the components in \p Mask are captured.
/// This assumes the pointer is to a function-local object. The caller is
/// responsible for ensuring this.
CaptureComponents PointerMayBeCaptured(
const Value *V, bool ReturnCaptures, CaptureComponents Mask,
function_ref<bool(CaptureComponents)> StopFn = capturesAnything,
Expand All @@ -64,6 +68,8 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
/// This assumes the pointer is to a function-local object. The caller is
/// responsible for ensuring this.
bool PointerMayBeCapturedBefore(const Value *V, bool ReturnCaptures,
const Instruction *I, const DominatorTree *DT,
bool IncludeI = false,
Expand Down Expand Up @@ -184,6 +190,8 @@ namespace llvm {
/// MaxUsesToExplore specifies how many uses the analysis should explore for
/// one value before giving up due too "too many uses". If MaxUsesToExplore
/// is zero, a default value is assumed.
/// This assumes the pointer is to a function-local object. The caller is
/// responsible for ensuring this.
void PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
unsigned MaxUsesToExplore = 0);

Expand Down
4 changes: 0 additions & 4 deletions llvm/lib/Analysis/CaptureTracking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -424,10 +424,6 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
if (MaxUsesToExplore == 0)
MaxUsesToExplore = DefaultMaxUsesToExplore;

// When analyzing a derived pointer, we need to analyze its underlying
// object to determine whether it is captured.
// E.g., `ptr + 1` is captured if `ptr` is captured.
V = getUnderlyingObjectAggressive(V);
SmallVector<const Use *, 20> Worklist;
Worklist.reserve(getDefaultMaxUsesToExploreForCaptureTracking());
SmallSet<const Use *, 20> Visited;
Expand Down
28 changes: 0 additions & 28 deletions llvm/unittests/Analysis/CaptureTrackingTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,31 +132,3 @@ TEST(CaptureTracking, MultipleUsesInSameInstruction) {
EXPECT_EQ(ICmp, CT.Captures[6]->getUser());
EXPECT_EQ(1u, CT.Captures[6]->getOperandNo());
}

TEST(CaptureTracking, DerivedPointerIfBasePointerCaptured) {
StringRef Assembly = R"(
declare void @bar(ptr)

define void @test() {
%stack_obj = alloca [2 x i32]
%derived = getelementptr inbounds [2 x i32], ptr %stack_obj, i64 0, i64 1
store i32 1, ptr %derived
call void @bar(ptr %stack_obj)
ret void
}
)";

LLVMContext Context;
SMDiagnostic Error;
auto M = parseAssemblyString(Assembly, Error, Context);
ASSERT_TRUE(M) << "Bad assembly?";

Function *F = M->getFunction("test");
BasicBlock *BB = &F->getEntryBlock();
Instruction *StackObj = &*BB->begin();
Instruction *DerivedPtr = StackObj->getNextNode();

// The base object and its derived pointer are both captured.
EXPECT_TRUE(PointerMayBeCaptured(StackObj, true));
EXPECT_TRUE(PointerMayBeCaptured(DerivedPtr, true));
}