Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Move p/invoke pregeneration out of single-exe branch #27585

Closed

Conversation

MichalStrehovsky
Copy link
Member

Brings over p/invoke pregeneration from CoreRT into crossgen2.

This is just xcopy of the src/tools/crossgen2 directory in the single-exe branch with David's profile data changes omitted. I had to fix a using directive in one of the files because a using keyword the branch was using got deleted in master.

This is basically all that I've done in #26767, #27036, #27109, #27286, #27389, #27436.

I don't know if there's a way to do this kind of selective merge in git, but I didn't particularly care - most of the interesting history for these files is on the CoreRT side anyway.

Also including the change to prestub.cpp that unlocks using the generated p/invokes.

Seems like getting rid of these being IL stubs will require RyuJIT changes and possibly R2R file format changes and I don't want to do those in a branch that has broken CI.

Cc @dotnet/crossgen-contrib

This is just xcopy of the src/tools/crossgen2 directory in the single-exe branch with David's profile data changes omitted. I had to fix a `using` directive in one of the files because a `using` keyword the branch was using got deleted in master.

I don't know if there's a way to do this kind of selective merge in git, but I don't particularly care. Most of the interesting history for these files is on the CoreRT side anyway.

Also including the change to prestub.cpp that unlocks using the generated p/invokes.
@MichalStrehovsky
Copy link
Member Author

@sergiy-k I'll leave this up to you whether we want to merge it now, or wait for me to come back. I validated this by running the Interop P/invoke tests, but I didn't get a chance to do a before/after with a complete Pri-1 test suite. It's a big change (most of it is not my code though).

I won't be back before we roll into the new repo, so we either merge this, or close this.

@sergiy-k
Copy link

sergiy-k commented Nov 4, 2019

@fadimounir and I talked about this. He will finish the necessary work and get the change merged. Thank you.

@fadimounir
Copy link

I validated this by running the Interop P/invoke tests, but I didn't get a chance to do a before/after with a complete Pri-1 test suite

I will validate the P1 tests. Any special command line or env variable to use or is the feature enabled by default?

Seems like getting rid of these being IL stubs will require RyuJIT changes and possibly R2R file format changes and I don't want to do those in a branch that has broken CI.

@MichalStrehovsky, I didn't fully understand this. What will require a JIT/R2R format change exactly? And is that a pre-requisite before merging this, or can I merge this once i validate the P1 tests with the changes?

@jkotas
Copy link
Member

jkotas commented Nov 4, 2019

I didn't fully understand this

I believe that this about fixing the IL stubs to be non-shared. The current version of the change has still them shared that comes with number of problems - if it works at all.

I think it would be fine to merge this change as is, except:

  • The change in prestub.cpp
  • The change that enables generation of the IL stubs in the compiler

Then work on enabling it properly (ie without the shared IL stubs) in follow up PR.

@MichalStrehovsky
Copy link
Member Author

Any special command line or env variable to use or is the feature enabled by default?

It's enabled when "input bubble" is enabled and CoreLib is part of the version bubble.

I believe that this about fixing the IL stubs to be non-shared.

Yup. My work in progress is here: MichalStrehovsky@ffc8fff. The importer.cpp change is incomplete - p/invoke inlining is still getting rejected a couple lines above the change in the commit (at !impCanPInvokeInlineCallSite(block)). I'm not sure whether that will be the way forward, but if I made control flow progress to that line by commenting out the check, things appeared to work.

@fadimounir
Copy link

I'm going to close this PR, since i can't make any updates to the changes. I created another copy of this PR, but without the changes in prestub.cpp and without change that emits the IL Stubs as @jkotas suggested.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants