EscapeAnalysis: make the use-point analysis more precise #27667
Conversation
In canEscapeToUsePoint only check the content node if it's a reference (see comment why this is needed). For all other node types, especially addresses, handle defer edges by propagating use-point infomation backward in the graph. This makes escape analysis more precise with address types, e.g. don't consider an inout address to escape to an apply if just the loaded value is passed to an apply argument.
|
@swift-ci test |
|
@swift-ci benchmark |
Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: -swiftlibs
How to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
There was a problem hiding this comment.
I want to see a unit test for the optimization(s) that you are unblocking. I need to know if the changes I want to make for robustness will interfere with that optimization. In this PR you only have incidental changes to the unit tests. It's not clear that those changes are essential to the test. So if someone changes anything later that breaks your expectations it won't be clear.
| // ... except it has defer predecessor edges, which are (potentially) not | ||
| // escaping: use-points are propagated in reverse direction along defer | ||
| // edges! | ||
| !Node->hasDeferPredecessors()) { |
There was a problem hiding this comment.
This appears to be fixing an existing correctness issue. Is that right?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| CGNode *ContentNode = ConGraph->getContentNode(Node); | ||
| if (ContentNode->escapesInsideFunction(false)) | ||
| return true; | ||
| if (V->getType().hasReferenceSemantics()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, checking the new flag what you'll add, will be better and more robust.
There was a problem hiding this comment.
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.
|
Thanks for reviewing! I'll add a test case which shows the optimization problem in a follow-up commit. |
In canEscapeToUsePoint only check the content node if it's a reference (see comment why this is needed).
For all other node types, especially addresses, handle defer edges by propagating use-point infomation backward in the graph.
This makes escape analysis more precise with address types, e.g. don't consider an inout address to escape to an apply if just the loaded value is passed to an apply argument.