-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
JIT: Simple importer single-block forward substitution #109482
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I guess this PR hits the same issues as Andy mentioned in #98380 (comment) from obvious issues is e.g. lack of "is address exposed" check. Also you sort of rely that def always comes before use in execution order |
Resolved.
Yes. If there's a use coming before the def like in a cycle, the def would be in another BB, in this case there won't be any substitution for the use as we restrict both the def and use to present in a single BB. |
2aa9a8c
to
74bbd1b
Compare
Tests are passing. This PR doesn't handle |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@hez2010 I think here (as with other recent proposed changes) we should not be adding new complexity to the importer, but refactoring inlining so it can happen opportunistically when we're able to propagate new information to a call site. So I recommend we close this. |
typeof
andFTN_ADDR
in importer.FTN_ADDR
as regular call to enable inliningThe branch name is
calli-inlining
but this is done mainly for enablingtypeof
propagation, whilefptr
propagation are enabled for free so I enabled inlining for them by the way.Example:
Before:
After:
Another example:
Diff for
Activator.CreateInstance<MyStruct>
: