Avoid copying out/inout args when inlining functions #4877
Merged
+80
−34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChrisDodd
force-pushed
the
cdodd-fninline
branch
from
c9cb66b
to
22951e3
Compare
fruffy
added
core
ChrisDodd
force-pushed
the
cdodd-fninline
branch
from
22951e3
to
9f8680b
Compare
ChrisDodd
force-pushed
the
cdodd-fninline
branch
from
9f8680b
to
599cfa1
Compare
ChrisDodd
force-pushed
the
cdodd-fninline
branch
3 times, most recently
from
89b945e
to
0da1153
Compare
|
While it would be nice to cover more cases, this is a useful start as it is. The main thing it does not catch is when a control calls a (global) function with a local or param of the control as an inout argument. That would be nice to catch, but would require determining that the control does not do something with that local/param that will result in it effectively being aliased to something else in the global function. |
asl
approved these changes
Sep 18, 2024
ChrisDodd
force-pushed
the
cdodd-fninline
branch
2 times, most recently
from
77eaad7
to
7e6ea8d
Compare
- If the actual argument for an out or inout argument is local to the calling functions, we can use it directly in the inlined function instead of introducing extra copies in and out. Signed-off-by: Chris Dodd <cdodd@nvidia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proposal/work in progress for a way of reducing the number of copies introduced when inlining.
On particularly problematic case is when you have a big struct (eg, all the headers) that is passed around to functions as an
inoutargument so they can access/modify it. Introducing a copy of all the headers is particularly inefficient, and tough to later optimize away if it was unnecessary.This change just has a minimal check -- if the actual argument passed to an
inoutoroutargument is local to the caller, it can't possibly be accessed by the callee directly, so no copy is needed -- the inlined code can just access it directly.A more general check would be to actually look at the callee -- if it does not access whatever is passed as an argument directly or indirectly, then no copy is needed. The indirectly part is complex -- requires looking recursively at whatever the callee calls, including any extern functions or methods on instances.
Or perhaps there should be a target-specific policy that controls this -- would allow better target-specific understanding of externs and what they do.
The problem is connected to doing too much in the frontend -- function inlining happens in the frontend (when arguably it should not), in order to allow inlining into type params (
bit<N>andint<N>in particular). If we remove the general inlining in the frontend (and only do it in the midend), it would greatly help with target-specific policies and tweaks.