Skip to content

EscapeAnalysis: make the use-point analysis more precise #27667

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.

Sign up for GitHub

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 1 commit into from
Oct 15, 2019
Merged

EscapeAnalysis: make the use-point analysis more precise #27667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 11 additions & 13 deletions include/swift/SILOptimizer/Analysis/EscapeAnalysis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,16 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
return false;
return true;
}

/// Returns true if the node has any predecessors with link to this node
/// with a defer-edge.
bool hasDeferPredecessors() const {
for (Predecessor Pred : Preds) {
if (Pred.getInt() == EdgeType::Defer)
return true;
}
return false;
}

friend class CGNodeMap;
friend class ConnectionGraph;
Expand Down Expand Up @@ -631,19 +641,7 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
}

/// Adds an argument/instruction in which the node's value is used.
int addUsePoint(CGNode *Node, SILNode *User) {
if (Node->getEscapeState() >= EscapeState::Global)
return -1;

User = User->getRepresentativeSILNodeInObject();
int Idx = (int)UsePoints.size();
assert(UsePoints.count(User) == 0 && "value is already a use-point");
UsePoints[User] = Idx;
UsePointTable.push_back(User);
assert(UsePoints.size() == UsePointTable.size());
Node->setUsePointBit(Idx);
return Idx;
}
int addUsePoint(CGNode *Node, SILNode *User);

/// Specifies that the node's value escapes to global or unidentified
/// memory.
Expand Down
62 changes: 46 additions & 16 deletions lib/SILOptimizer/Analysis/EscapeAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,27 @@ updatePointsTo(CGNode *InitialNode, CGNode *pointsTo) {
clearWorkListFlags(WorkList);
}

int EscapeAnalysis::ConnectionGraph::addUsePoint(CGNode *Node, SILNode *User) {
// For escaping nodes we have to make worst case assumptions anyway. So no
// need to store use point information.
if (Node->getEscapeState() >= EscapeState::Global &&
// ... except it has defer predecessor edges, which are (potentially) not
// escaping: use-points are propagated in reverse direction along defer
// edges!
!Node->hasDeferPredecessors()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be fixing an existing correctness issue. Is that right?

Copy link
Contributor Author

@eeckstein eeckstein Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No, There was not correctness issue.
There is (better: was) no point in adding use-points for escaping nodes because escaping nodes would cause to bail anyway. Only now, when propagating in reverse direction, we have to consider this, because a predecessor might not be escaping even if the node itself is escaping.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're saying. But if the direction of the defer edge matters, then I think phi arguments needed defer edges in both directions. Or, we just say that the direction doesn't matter and propagate both escape states and use points in both directions.

If the direction does not matter, then we could probably just merge the nodes in the defer web.

return -1;
}

User = User->getRepresentativeSILNodeInObject();
int Idx = (int)UsePoints.size();
assert(UsePoints.count(User) == 0 && "value is already a use-point");
UsePoints[User] = Idx;
UsePointTable.push_back(User);
assert(UsePoints.size() == UsePointTable.size());
Node->setUsePointBit(Idx);
return Idx;
}

void EscapeAnalysis::ConnectionGraph::propagateEscapeStates() {
bool Changed = false;
do {
Expand Down Expand Up @@ -449,6 +470,10 @@ void EscapeAnalysis::ConnectionGraph::computeUsePoints() {
for (CGNode *Def : Node->defersTo) {
Changed |= Def->mergeUsePoints(Node);
}
for (Predecessor Pred : Node->Preds) {
if (Pred.getInt() == EdgeType::Defer)
Changed |= Pred.getPointer()->mergeUsePoints(Node);
}
}
} while (Changed);
}
Expand Down Expand Up @@ -1858,23 +1883,28 @@ bool EscapeAnalysis::canEscapeToUsePoint(SILValue V, SILNode *UsePoint,
if (ConGraph->isUsePoint(UsePoint, Node))
return true;

assert(isPointer(V) && "should not have a node for a non-pointer");

// Check if the object "content" can escape to the called function.
// This will catch cases where V is a reference and a pointer to a stored
// property escapes.
// It's also important in case of a pointer assignment, e.g.
// V = V1
// apply(V1)
// In this case the apply is only a use-point for V1 and V1's content node.
// As V1's content node is the same as V's content node, we also make the
// check for the content node.
CGNode *ContentNode = ConGraph->getContentNode(Node);
if (ContentNode->escapesInsideFunction(false))
return true;
if (V->getType().hasReferenceSemantics()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this check is fine for now. I don't think it's robust to assume here that all other pointsTo edges represent physical indirection (which is assumed by the caller of this API) when that fact isn't represented anywhere in the graph. That assumes something about how the graph is constructed and assumes that nothing unforeseen has happened to the graph in the mean time. In debugging escape graphs, I've noticed many scenarios that I never foresaw.

Very soon I can commit a change that marks nodes that start a heap object. I'm not sure that will be sufficient to handle whatever case you need optimized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, checking the new flag what you'll add, will be better and more robust.

Copy link
Contributor

Choose a reason for hiding this comment

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

The basic problem with this API is that the client has some specification for an "object" that doesn't match any concept in the ConnectionGraph. This is one of the key issues that I was addressing on my working branch before I put it on hold to work on bugs and benchmarks.

Can you tell me what test broke with this change so I can see if I'm also breaking it?

I wonder if it would be sufficient for you to simply do this check instead:
If the SILValue's node is a 'Value' node type, then it can't represent the content of a heap object. So, in that case, you should never need to look at it's content node.

When checking for a reference type. I'm afraid that getUnderlyingObject might look through an address to reference cast.

// In case V is the result of getUnderlyingObject, we also have to check
// the content node. Example:
//
// %a = ref_element %r
// apply %f(%a)
//
// The reference %r does not actually escape to the function call. But in
// case this API is called like
//
// canEscapeToUsePoint(getUnderlyingObject(applyArgument))
//
// it would yield false, although the projected object address is passed to
// the function.

CGNode *ContentNode = ConGraph->getContentNode(Node);
if (ContentNode->escapesInsideFunction(false))
return true;

if (ConGraph->isUsePoint(UsePoint, ContentNode))
return true;
if (ConGraph->isUsePoint(UsePoint, ContentNode))
return true;
}

return false;
}
Expand Down
5 changes: 3 additions & 2 deletions lib/SILOptimizer/Transforms/StackPromotion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,9 @@ bool StackPromotion::tryPromoteAlloc(AllocRefInst *ARI, EscapeAnalysis *EA,
if (SILInstruction *I = dyn_cast<SILInstruction>(UsePoint)) {
UsePoints.push_back(I);
} else {
// Also block arguments can be use points.
SILBasicBlock *UseBB = cast<SILPhiArgument>(UsePoint)->getParent();
// Also block arguments can be use points (even function arguments, as
// use-points are propagated backwards along defer edges).
SILBasicBlock *UseBB = cast<SILArgument>(UsePoint)->getParent();
// For simplicity we just add the first instruction of the block as use
// point.
UsePoints.push_back(&UseBB->front());
Expand Down
24 changes: 12 additions & 12 deletions test/SILOptimizer/escape_analysis.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ bb0(%0 : $*Y, %1 : $X):
// CHECK-LABEL: CG of deferringEdge
// CHECK-NEXT: Arg %0 Esc: A, Succ: (%3.1)
// CHECK-NEXT: Arg %1 Esc: A, Succ:
// CHECK-NEXT: Val %3 Esc: %3, Succ: (%3.1), %0
// CHECK-NEXT: Val %3 Esc: %0,%3, Succ: (%3.1), %0
// CHECK-NEXT: Con %3.1 Esc: A, Succ: (%3.2)
// CHECK-NEXT: Con %3.2 Esc: A, Succ: %1
// CHECK-NEXT: Ret Esc: R, Succ: %0
Expand Down Expand Up @@ -153,8 +153,8 @@ bb0:
// CHECK-NEXT: Val %1 Esc: A, Succ: (%1.1)
// CHECK-NEXT: Con %1.1 Esc: A, Succ: (%11.1)
// CHECK-NEXT: Val %4 Esc: A, Succ: (%1.1)
// CHECK-NEXT: Val %7 Esc: %11, Succ: (%1.1)
// CHECK-NEXT: Val %11 Esc: %11, Succ: (%1.1), %7, %11.1
// CHECK-NEXT: Val %7 Esc: %0,%11, Succ: (%1.1)
// CHECK-NEXT: Val %11 Esc: %0,%11, Succ: (%1.1), %7, %11.1
// CHECK-NEXT: Con %11.1 Esc: A, Succ: (%1.1), %0, %1, %4
// CHECK-NEXT: Ret Esc: R, Succ: %11.1
// CHECK-NEXT: End
Expand Down Expand Up @@ -212,7 +212,7 @@ bb0(%0 : $LinkedNode):

// CHECK-LABEL: CG of loadNext
// CHECK-NEXT: Arg %0 Esc: A, Succ: (%2.1)
// CHECK-NEXT: Val %2 Esc: %2, Succ: (%2.1), %0, %2.2
// CHECK-NEXT: Val %2 Esc: %0,%2, Succ: (%2.1), %0, %2.2
// CHECK-NEXT: Con %2.1 Esc: A, Succ: (%2.2)
// CHECK-NEXT: Con %2.2 Esc: A, Succ: (%2.1)
// CHECK-NEXT: Ret Esc: R, Succ: %2.2
Expand Down Expand Up @@ -406,12 +406,12 @@ sil @call_copy_addr_content : $@convention(thin) () -> () {
// CHECK-LABEL: CG of test_partial_apply
// CHECK-NEXT: Arg %1 Esc: G, Succ:
// CHECK-NEXT: Arg %2 Esc: A, Succ: (%6.3)
// CHECK-NEXT: Val %3 Esc: %14,%15,%17, Succ: (%6.1)
// CHECK-NEXT: Val %6 Esc: %14,%15,%16, Succ: (%6.1)
// CHECK-NEXT: Val %3 Esc: %14,%15,%16,%17, Succ: (%6.1)
// CHECK-NEXT: Val %6 Esc: %14,%15,%16,%17, Succ: (%6.1)
// CHECK-NEXT: Con %6.1 Esc: %14,%15,%16,%17, Succ: (%6.2)
// CHECK-NEXT: Con %6.2 Esc: %14,%15,%16,%17, Succ: %2
// CHECK-NEXT: Con %6.2 Esc: %2,%14,%15,%16,%17, Succ: %2
// CHECK-NEXT: Con %6.3 Esc: G, Succ:
// CHECK-NEXT: Val %12 Esc: %14,%15, Succ: %3, %6
// CHECK-NEXT: Val %12 Esc: %14,%15,%16,%17, Succ: %3, %6
// CHECK-NEXT: End
sil @test_partial_apply : $@convention(thin) (Int64, @owned X, @owned Y) -> Int64 {
bb0(%0 : $Int64, %1 : $X, %2 : $Y):
Expand Down Expand Up @@ -443,7 +443,7 @@ bb0(%0 : $Int64, %1 : $X, %2 : $Y):
// CHECK-NEXT: Con %2.1 Esc: A, Succ: (%2.2)
// CHECK-NEXT: Con %2.2 Esc: A, Succ: (%2.3)
// CHECK-NEXT: Con %2.3 Esc: G, Succ:
// CHECK-NEXT: Val %7 Esc: %8, Succ: %2
// CHECK-NEXT: Val %7 Esc: %2,%8, Succ: %2
// CHECK-NEXT: End
sil @closure1 : $@convention(thin) (@owned X, @owned <τ_0_0> { var τ_0_0 } <Int64>, @owned <τ_0_0> { var τ_0_0 } <Y>) -> Int64 {
bb0(%0 : $X, %1 : $<τ_0_0> { var τ_0_0 } <Int64>, %2 : $<τ_0_0> { var τ_0_0 } <Y>):
Expand Down Expand Up @@ -761,7 +761,7 @@ sil @unknown_throwing_func : $@convention(thin) (@owned X) -> (@owned X, @error
// CHECK-NEXT: Arg %0 Esc: A, Succ: (%1.3)
// CHECK-NEXT: Val %1 Esc: %6, Succ: (%1.1)
// CHECK-NEXT: Con %1.1 Esc: %6, Succ: (%1.2)
// CHECK-NEXT: Con %1.2 Esc: %6, Succ: %0
// CHECK-NEXT: Con %1.2 Esc: %0,%6, Succ: %0
// CHECK-NEXT: Con %1.3 Esc: G, Succ:
// CHECK-NEXT: Val %5 Esc: %6, Succ: %1
// CHECK-NEXT: End
Expand Down Expand Up @@ -969,7 +969,7 @@ bb0(%0 : $Builtin.Int64, %1 : $X, %2 : $X, %3 : $X):
// CHECK-NEXT: Arg %0 Esc: A, Succ:
// CHECK-NEXT: Val %1 Esc: , Succ: (%1.1)
// CHECK-NEXT: Con %1.1 Esc: , Succ: (%1.2)
// CHECK-NEXT: Con %1.2 Esc: , Succ: %0
// CHECK-NEXT: Con %1.2 Esc: %0, Succ: %0
// CHECK-NEXT: End
sil @test_existential_addr : $@convention(thin) (@owned Pointer) -> () {
bb0(%0 : $Pointer):
Expand Down Expand Up @@ -1181,7 +1181,7 @@ bb0(%0 : $*Array<X>):
// CHECK-LABEL: CG of arraysemantics_get_element_address
// CHECK-NEXT: Arg %0 Esc: A, Succ: (%0.1)
// CHECK-NEXT: Con %0.1 Esc: A, Succ:
// CHECK-NEXT: Val %4 Esc: , Succ: %0.1
// CHECK-NEXT: Val %4 Esc: %0,%4, Succ: %0.1
// CHECK-NEXT: End
sil @arraysemantics_get_element_address : $@convention(thin) (Array<X>) -> () {
bb0(%0 : $Array<X>):
Expand Down