Call targets should be functions instead of global variables
AndyAyersMS opened this issue · 7 comments
When creating symbolic representations of known function addresses we should use function global objects instead of global variables.
For instance, something like this should be readily recognized as a tail call.
@"Test.Fact.factTR(TK_6000001)" = external constant i64
define i32 @Test.Fact.factTR(i32 %param0, i32 %param1) gc "coreclr" {
entry:
... (snip)
%11 = call i32 bitcast (i64* @"Test.Fact.factTR(TK_6000001)" to i32 (i32, i32)*)(i32 %7, i32 %10), !dbg !16
ret i32 %11, !dbg !14
There was some discussion of this in #690. It's buried in "comments on outdated diffs", so I'll copy it here:
Should we be using Functions instead of GlobalVariables for these? (I'm not trying to say we should, just wondering what your thoughts are. On the one hand, with Functions we could apply function-level attributes that analyses might pick up on; on the other hand, these really are pretty opaque to us so maybe it's safer to model them as indirect calls via function pointer global variables.)
There are some practical difficulties with using Functions here. Functions require the correct FunctionType when they are created and we don't have one yet. If we find a very good reason to use Functions that can probably be changed but it's not a trivial change
Sounds like tail recursion may be the motivating case. (Though if it's really just that we could consider hacking around it by recognizing self-references in the call target processing code...)
Inlining is probably a stronger motivation. I suppose we can keep a side table if we have to, but it seems like we ought to be able to describe what we need in the IR directly.
Inlining is probably a stronger motivation
Won't we need the IR for the inline candidate anyway? Or are you suggesting that the IR changes that will be necessary if we decide to import a call target for inlining will be easier if the target was already defined as a Function?
FWIW, it does seem like we should be able to use extern functions rather than global variables.
it seems like we ought to be able to describe what we need in the IR directly
...
it does seem like we should be able to use extern functions rather than global variables
Right, nobody's arguing that the IR or MSIL aren't expressive enough to hook up a logical representation using Functions.
The issue is simply that the reader code is factored in such a way that the place where we have a token and need to build up an IR entity knows that it has a token but doesn't (as it's currently written) have the context of that token being a function with a specific signature; I believe that context is buried in a continuation a few frames up the call stack, and represented differently for helper calls vs IL callees. Plumbing through the context, or returning some sentinel construct that later gets remapped to a Function, is doable and good, it's just a matter of engineering.
I believe that context is buried in a continuation a few frames up the call stack
It's not just that; the signature from the EE is not enough. We calculate the final signature much later in ABICallSignature::emitCall. I think the correct way to handle this is to postpone Function creation until we are ready to emit the call. There is no reason to create it as early as we do today.
That's more or less what I was thinking:
- Update the map to hold GlobalObjects, adjust clients accordingly
- Map the current function when reading starts, since we know its updated signature.
- When we know we have a call target, we lookup/create a function instead of a global var like we do now.
- Handle any downstream fixup processing fallout.
Implemented more or less the above, albeit with eager creation and subsequent repair.
Closed via #944