Skip to content

Commit 2daf1d2

Browse files
committed
[opt-remark] When looking for debug_value users, look modulo RC Identity preserving users.
A key concept in late ARC optimization is "RC Identity". In short, a result of an instruction is rc-identical to an operand of the instruction if one can safely move a retain (release) from before the instruction on the result to one after on the operand without changing the program semantics. This creates a simple model where one can work on equivalence classes of rc-identical values (using a dominating definition generally as the representative) and thus optimize/pair retain, release. When preparing for late ARC optimization, the optimizer will normalize aggregate ARC operations (retain_value, release_value) into singular strong_retain, strong_release operations on leaf types of the aggregate that are non-trivial. As an example, a retain_value on a KlassPair would be canonicalized into two strong_retain, one for the lhs and one for the rhs. When this is done, the optimizer generally just creates new struct_extract at the point where the retain is. In such a case, we may have that the debug_value for the underlying type is actually on a reformed aggregate whose underlying parts we are retaining: ``` bb0(%0 : $Builtin.NativeObject): strong_retain %0 %1 = struct $Array(%0 : $Builtin.NativeObject, ...) debug_value %1 : $Array, ... ``` By looking through RC identical uses, we can handle a large subset of these cases without much effort: ones were there is a single owning pointer like Array. To handle more complex cases we would have to calculate an inverse access path needed to get back to our value and somehow deal with all of the complexity therein (I am sure we can do it I just haven't thought through all of the details). The only interesting behavior that this results in is that when we emit diagnostics, we just use the rc-identical transitive use debug_value's name without a projection path. This is because the source location associated with that debug_value is with a separate value that is rc-identical to the actual value that we visited during our opt-remark traversal up the def-use graph. Consider the following example below, noting the comments that show in the SIL itself what I attempted to explain above. ``` struct KlassPair { var lhs: Klass var rhs: Klass } struct StateWithOwningPointer { var state: TrivialState var owningPtr: Klass } sil @thefunction : $@convention(thin) () -> () { bb0: %0 = apply %getKlassPair() : $@convention(thin) () -> @owned KlassPair // This debug_value's name can be combined... debug_value %0 : $KlassPair, name "myPair" // ... with the access path from the struct_extract here... %1 = struct_extract %0 : $KlassPair, #KlassPair.lhs // ... to emit a nice diagnostic that 'myPair.lhs' is being retained. strong_retain %1 : $Klass // In contrast in the case below, we rely on looking through rc-identity uses // to find the debug_value. In this case, the source info associated with the // debug_value (%2) is no longer associated with the underlying access path we // have been tracking upwards (%1 is in our access path list). Instead, we // know that the debug_value is rc-identical to whatever value we were // originally tracking up (%1) and thus the correct identifier to use is the // direct name of the identifier alone (without access path) since that source // identifier must be some value in the source that by itself is rc-identical // to whatever is being manipulated. Thus if we were to emit the access path // here for na rc-identical use we would get "myAdditionalState.owningPtr" // which is misleading since ArrayWrapperWithMoreState does not have a field // named 'owningPtr', its subfield array does. That being said since // rc-identity means a retain_value on the value with the debug_value upon it // is equivalent to the access path value we found by walking up the def-use // graph from our strong_retain's operand. %0a = apply %getStateWithOwningPointer() : $@convention(thin) () -> @owned StateWithOwningPointer %1 = struct_extract %0a : $StateWithOwningPointer, #StateWithOwningPointer.owningPtr strong_retain %1 : $Klass %2 = struct $Array(%0 : $Builtin.NativeObject, ...) %3 = struct $ArrayWrapperWithMoreState(%2 : $Array, %moreState : MoreState) debug_value %2 : $ArrayWrapperWithMoreState, name "myAdditionalState" } ```
1 parent a12963a commit 2daf1d2

File tree

4 files changed

+206
-90
lines changed

4 files changed

+206
-90
lines changed

include/swift/SILOptimizer/Analysis/RCIdentityAnalysis.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,15 +55,19 @@ class RCIdentityFunctionInfo {
5555
/// *NOTE* This ignores obvious ARC escapes where the a potential
5656
/// user of the RC is not managed by ARC. For instance
5757
/// unchecked_trivial_bit_cast.
58-
void getRCUses(SILValue V, llvm::SmallVectorImpl<Operand *> &Uses);
58+
void getRCUses(SILValue V, SmallVectorImpl<Operand *> &Uses);
5959

6060
/// A helper method that calls getRCUses and then maps each operand to the
6161
/// operands user and then uniques the list.
6262
///
6363
/// *NOTE* The routine asserts that the passed in Users array is empty for
6464
/// simplicity. If needed this can be changed, but it is not necessary given
6565
/// current uses.
66-
void getRCUsers(SILValue V, llvm::SmallVectorImpl<SILInstruction *> &Users);
66+
void getRCUsers(SILValue V, SmallVectorImpl<SILInstruction *> &Users);
67+
68+
/// Like getRCUses except uses a callback to prevent the need for an
69+
/// intermediate array.
70+
void visitRCUses(SILValue V, function_ref<void(Operand *)> Visitor);
6771

6872
void handleDeleteNotification(SILNode *node) {
6973
auto value = dyn_cast<ValueBase>(node);

lib/SILOptimizer/Analysis/RCIdentityAnalysis.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -522,13 +522,18 @@ void RCIdentityFunctionInfo::getRCUsers(
522522
/// We only use the instruction analysis here.
523523
void RCIdentityFunctionInfo::getRCUses(SILValue InputValue,
524524
llvm::SmallVectorImpl<Operand *> &Uses) {
525+
return visitRCUses(InputValue,
526+
[&](Operand *op) { return Uses.push_back(op); });
527+
}
525528

529+
void RCIdentityFunctionInfo::visitRCUses(
530+
SILValue InputValue, function_ref<void(Operand *)> Visitor) {
526531
// Add V to the worklist.
527-
llvm::SmallVector<SILValue, 8> Worklist;
532+
SmallVector<SILValue, 8> Worklist;
528533
Worklist.push_back(InputValue);
529534

530535
// A set used to ensure we only visit uses once.
531-
llvm::SmallPtrSet<Operand *, 8> VisitedOps;
536+
SmallPtrSet<Operand *, 8> VisitedOps;
532537

533538
// Then until we finish the worklist...
534539
while (!Worklist.empty()) {
@@ -564,7 +569,7 @@ void RCIdentityFunctionInfo::getRCUses(SILValue InputValue,
564569
}
565570

566571
// Otherwise, stop searching and report this RC operand.
567-
Uses.push_back(Op);
572+
Visitor(Op);
568573
}
569574
}
570575
}

lib/SILOptimizer/Transforms/OptRemarkGenerator.cpp

Lines changed: 135 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ struct ValueToDeclInferrer {
6262

6363
RCIdentityFunctionInfo &rcfi;
6464
SmallVector<std::pair<SILType, Projection>, 32> accessPath;
65+
SmallVector<Operand *, 32> rcIdenticalSecondaryUseSearch;
6566

6667
ValueToDeclInferrer(RCIdentityFunctionInfo &rcfi) : rcfi(rcfi) {}
6768

@@ -70,17 +71,74 @@ struct ValueToDeclInferrer {
7071
bool infer(ArgumentKeyKind keyKind, SILValue value,
7172
SmallVectorImpl<Argument> &resultingInferredDecls);
7273

73-
/// Print out a note to \p stream that beings at decl and then consumes the
74-
/// accessPath we computed for decl producing a segmented access path, e.x.:
75-
/// "of 'x.lhs.ivar'".
76-
void printNote(llvm::raw_string_ostream &stream, const ValueDecl *decl);
74+
/// Print out a note to \p stream that beings at decl and then if
75+
/// useProjectionPath is set to true iterates the accessPath we computed for
76+
/// decl producing a segmented access path, e.x.: "of 'x.lhs.ivar'".
77+
///
78+
/// The reason why one may not want to emit a projection path note here is if
79+
/// one found an debug_value on a value that is rc-identical to the actual
80+
/// value associated with the current projection path. Consider the following
81+
/// SIL:
82+
///
83+
/// struct KlassPair {
84+
/// var lhs: Klass
85+
/// var rhs: Klass
86+
/// }
87+
///
88+
/// struct StateWithOwningPointer {
89+
/// var state: TrivialState
90+
/// var owningPtr: Klass
91+
/// }
92+
///
93+
/// sil @theFunction : $@convention(thin) () -> () {
94+
/// bb0:
95+
/// %0 = apply %getKlassPair() : $@convention(thin) () -> @owned KlassPair
96+
/// // This debug_value's name can be combined...
97+
/// debug_value %0 : $KlassPair, name "myPair"
98+
/// // ... with the access path from the struct_extract here...
99+
/// %1 = struct_extract %0 : $KlassPair, #KlassPair.lhs
100+
/// // ... to emit a nice diagnostic that 'myPair.lhs' is being retained.
101+
/// strong_retain %1 : $Klass
102+
///
103+
/// // In contrast in this case, we rely on looking through rc-identity
104+
/// // uses to find the debug_value. In this case, the source info
105+
/// // associated with the debug_value (%2) is no longer associated with
106+
/// // the underlying access path we have been tracking upwards (%1 is in
107+
/// // our access path list). Instead, we know that the debug_value is
108+
/// // rc-identical to whatever value we were originally tracking up (%1)
109+
/// // and thus the correct identifier to use is the direct name of the
110+
/// // identifier alone since that source identifier must be some value
111+
/// // in the source that by itself is rc-identical to whatever is being
112+
/// // manipulated.
113+
/// //
114+
/// // The reason why we must do this is due to the behavior of the late
115+
/// // optimizer and how it forms these patterns in the code.
116+
/// %0a = apply %getStateWithOwningPointer() : $@convention(thin) () -> @owned StateWithOwningPointer
117+
/// %1 = struct_extract %0a : $StateWithOwningPointer, #StateWithOwningPointer.owningPtr
118+
/// strong_retain %1 : $Klass
119+
/// %2 = struct $Array(%0 : $Builtin.NativeObject, ...)
120+
/// debug_value %2 : $Array, ...
121+
/// }
122+
void printNote(llvm::raw_string_ostream &stream, StringRef name,
123+
bool shouldPrintAccessPath = true);
124+
125+
/// Convenience overload that calls:
126+
///
127+
/// printNote(stream, decl->getBaseName().userFacingName(), shouldPrintAccessPath).
128+
void printNote(llvm::raw_string_ostream &stream, const ValueDecl *decl,
129+
bool shouldPrintAccessPath = true) {
130+
printNote(stream, decl->getBaseName().userFacingName(),
131+
shouldPrintAccessPath);
132+
}
77133

78-
void printProjectionPath(llvm::raw_string_ostream &stream);
134+
/// Print out non-destructively the current access path we have found to
135+
/// stream.
136+
void printAccessPath(llvm::raw_string_ostream &stream);
79137
};
80138

81139
} // anonymous namespace
82140

83-
void ValueToDeclInferrer::printProjectionPath(llvm::raw_string_ostream &stream) {
141+
void ValueToDeclInferrer::printAccessPath(llvm::raw_string_ostream &stream) {
84142
for (auto &pair : accessPath) {
85143
auto baseType = pair.first;
86144
auto &proj = pair.second;
@@ -120,11 +178,11 @@ void ValueToDeclInferrer::printProjectionPath(llvm::raw_string_ostream &stream)
120178
}
121179

122180
void ValueToDeclInferrer::printNote(llvm::raw_string_ostream &stream,
123-
const ValueDecl *decl) {
124-
assert(decl &&
125-
"We assume for now that this is always called with a non-null decl");
126-
stream << "of '" << decl->getBaseName();
127-
printProjectionPath(stream);
181+
StringRef name,
182+
bool shouldPrintAccessPath) {
183+
stream << "of '" << name;
184+
if (shouldPrintAccessPath)
185+
printAccessPath(stream);
128186
stream << "'";
129187
}
130188

@@ -161,6 +219,7 @@ bool ValueToDeclInferrer::infer(
161219
SWIFT_DEFER {
162220
accessPath.clear();
163221
};
222+
SmallPtrSet<SILInstruction *, 8> visitedDebugValueInsts;
164223

165224
// This is a linear IR traversal using a 'falling while loop'. That means
166225
// every time through the loop we are trying to handle a case before we hit
@@ -233,62 +292,77 @@ bool ValueToDeclInferrer::infer(
233292
}
234293
}
235294

236-
// Then visit our users and see if we can find a debug_value that provides
237-
// us with a decl we can use to construct an argument.
295+
// Then visit our users (ignoring rc identical transformations) and see if
296+
// we can find a debug_value that provides us with a decl we can use to
297+
// construct an argument.
298+
//
299+
// The reason why we do this is that sometimes we reform a struct from its
300+
// constituant parts and then construct the debug_value from that. For
301+
// instance, if we FSOed.
238302
bool foundDeclFromUse = false;
239-
for (auto *use : value->getUses()) {
303+
rcfi.visitRCUses(value, [&](Operand *use) {
240304
// Skip type dependent uses.
241305
if (use->isTypeDependent())
242-
continue;
306+
return;
307+
308+
// Then see if we have a debug_value that is associated with a non-inlined
309+
// debug scope. Such an instruction is an instruction that is from the
310+
// current function.
311+
auto *dvi = dyn_cast<DebugValueInst>(use->getUser());
312+
if (!dvi || !hasNonInlinedDebugScope(dvi))
313+
return;
243314

244-
if (auto *dvi = dyn_cast<DebugValueInst>(use->getUser())) {
245-
// Check if our debug_value has a decl and was not inlined into the
246-
// current function.
247-
if (hasNonInlinedDebugScope(dvi)) {
248-
if (auto *decl = dvi->getDecl()) {
249-
std::string msg;
250-
{
251-
llvm::raw_string_ostream stream(msg);
252-
printNote(stream, decl);
253-
}
254-
resultingInferredDecls.emplace_back(
255-
OptRemark::ArgumentKey{keyKind, "InferredValue"},
256-
std::move(msg), decl);
257-
foundDeclFromUse = true;
258-
} else {
259-
// If we did not have a decl, see if we were asked for testing
260-
// purposes to use SILDebugInfo to create a placeholder inferred
261-
// value.
262-
if (DecllessDebugValueUseSILDebugInfo) {
263-
if (auto varInfo = dvi->getVarInfo()) {
264-
auto name = varInfo->Name;
265-
if (!name.empty()) {
266-
std::string msg;
267-
{
268-
llvm::raw_string_ostream stream(msg);
269-
stream << "of '" << name;
270-
printProjectionPath(stream);
271-
stream << "'";
272-
}
273-
resultingInferredDecls.push_back(
274-
Argument({keyKind, "InferredValue"},
275-
std::move(msg),
276-
dvi->getLoc()));
277-
foundDeclFromUse = true;
278-
}
279-
}
280-
}
281-
}
315+
// See if we have already inferred this debug_value as a potential source
316+
// for this instruction. In such a case, just return.
317+
if (!visitedDebugValueInsts.insert(dvi).second)
318+
return;
319+
320+
if (auto *decl = dvi->getDecl()) {
321+
std::string msg;
322+
{
323+
llvm::raw_string_ostream stream(msg);
324+
// If we are not a top level use, we must be a rc-identical transitive
325+
// use. In such a case, we just print out the rc identical value
326+
// without a projection path. This is because we now have a better
327+
// name and the name is rc-identical to whatever was at the end of the
328+
// projection path but is not at the end of that projection path.
329+
printNote(stream, decl,
330+
use->get() == value /*print projection path*/);
282331
}
332+
resultingInferredDecls.emplace_back(
333+
OptRemark::ArgumentKey{keyKind, "InferredValue"}, std::move(msg),
334+
decl);
335+
foundDeclFromUse = true;
336+
return;
283337
}
284-
}
285338

286-
// At this point, we could not infer any argument. See if we can look
287-
// through loads.
288-
//
289-
// TODO: Add GEPs to construct a ProjectionPath.
339+
// If we did not have a decl, see if we were asked for testing
340+
// purposes to use SILDebugInfo to create a placeholder inferred
341+
// value.
342+
if (!DecllessDebugValueUseSILDebugInfo)
343+
return;
344+
345+
auto varInfo = dvi->getVarInfo();
346+
if (!varInfo)
347+
return;
348+
349+
auto name = varInfo->Name;
350+
if (name.empty())
351+
return;
352+
353+
std::string msg;
354+
{
355+
llvm::raw_string_ostream stream(msg);
356+
printNote(stream, name, use->get() == value /*print projection path*/);
357+
}
358+
resultingInferredDecls.push_back(
359+
Argument({keyKind, "InferredValue"}, std::move(msg), dvi->getLoc()));
360+
foundDeclFromUse = true;
361+
});
290362

291-
// Finally, see if we can look through a load...
363+
// At this point, we could not infer any argument. See if we can look up the
364+
// def-use graph and come up with a good location after looking through
365+
// loads and projections.
292366
if (auto *li = dyn_cast<LoadInst>(value)) {
293367
value = stripAccessMarkers(li->getOperand());
294368
continue;
@@ -305,7 +379,8 @@ bool ValueToDeclInferrer::infer(
305379
// TODO: We could emit at this point a msg for temporary allocations.
306380

307381
// If we reached this point, we finished falling through the loop and return
308-
// true.
382+
// if we found any decls from uses. We always process everything so we /can/
383+
// potentially emit multiple diagnostics.
309384
return foundDeclFromUse;
310385
}
311386
}

0 commit comments

Comments
 (0)