SIL: Replace uses of getReferencedFunction() by getReferencedFunction… #25073
Conversation
…OrNull() and getInitialReferencedFunction() With the advent of dynamic_function_ref the actual callee of such a ref my vary. Optimizations should not assume to know the content of a function referenced by dynamic_function_ref. Introduce getReferencedFunctionOrNull which will return null for such function refs. And getInitialReferencedFunction to return the referenced function. Use as appropriate. rdar://50959798
|
@swift-ci Please test |
|
|
||
| /// Return the referenced function if the callee is a function_ref like | ||
| /// instruction. | ||
| /// WARNING: This not necessarily the function that will be called at runtime. |
There was a problem hiding this comment.
Could you put a newline here before, WARNING.
include/swift/SIL/ApplySite.h
Outdated
| /// Return the referenced function if the callee is a function_ref like | ||
| /// instruction. | ||
| /// WARNING: This not necessarily the function that will be called at runtime. | ||
| /// If the callee is a (prev_)dynamic_function_ref the actuall function called |
|
|
||
| /// Return the referenced function if the callee is a function_ref like | ||
| /// instruction. | ||
| /// WARNING: This not necessarily the function that will be called at runtime. |
There was a problem hiding this comment.
Same spelling issue here.
include/swift/SIL/SILInstruction.h
Outdated
| /// Return the referenced function. | ||
| SILFunction *getReferencedFunction() const { return f; } | ||
| /// WARNING: This not necessarily the function that will be called at runtime. | ||
| /// If this is a (prev_)dynamic_function_ref the actuall function called might |
|
Overall, I find the names here to be sort of confusing. When do I want to use getReferencedFunction and when do I want to use getInitialFunction? When I think at a high level about it, I think the distinction is about the actual function to be called vs the identity of a specific abstract function pointer. I don't have a better name at this point in time, but these names worry me. |
|
That being said, if you need to get this in ASAP, we can talk about this offline and follow up. |
|
You want to use getReferencedFunction when the client cares about the dynamic target - the content of the function. The higher level API named the same way on ApplySite and apply instructions already had this behavior: it returned the function_refed sil function or null if the callee was dynamic. How about something like: I liked the initially referenced naming because to me when I read this I think: initially means something might change ... I am totally open to a different naming scheme. |
|
If you think about the essence of what you are distinguishing, one is the dynamic function at runtime and the other is in a sense an ID for a specific function ptr in the program. The initial function is the identity of that abstract concept. I wonder if we can work that into the names somehow. |
|
if you need to get this in ASAP, we can always rename using Xcode's refactoring engine. |
|
There is the distinction (that I think you pointed out earlier): getStaticallyReferencedFunction() vs getDynamicallyReferencedFunction() but I feel like getStaticallyReferenced does not sound scary enough when read alone in the context of some code. “Initially” conveys that something might change. getKnownDynamicallyReferenced() Vs getInitiallyStaticallyReferenced() Maybe? |
|
That's the primary API. My main suggestion here is to add more comments to the WARNING so it's clear that this API is deprecated and the alternative is to use the referenced SILFunctionType instead. Maybe even a FIXME to rewrite the current uses of that API. Do you foresee any difficulty rewriting those uses over time? |
|
Just had an interesting thought. One thing I noticed while reading the code is that the major places that the getInitiallyReferencedFunction() are cloner like functions. Maybe the right way to do this is to make it so that the dynamic function ref takes a static function ref as its "default value". Then when we clone, the inst will just clone itself and the clone the function_ref which would go through the normal code path. |
|
Yes there are placed like the cloner, verifier, and SILFunction ref counting where it won’t go away. If we want to make it really hard to use the API, we could make the API private and friend those places. |
|
Instead of |
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
…OrNull() and getInitialReferencedFunction()
With the advent of dynamic_function_ref the actual callee of such a ref
my vary. Optimizations should not assume to know the content of a
function referenced by dynamic_function_ref. Introduce
getReferencedFunctionOrNull which will return null for such function
refs. And getInitialReferencedFunction to return the referenced
function.
Use as appropriate.
rdar://50959798