Skip to content
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

AOT ILC using a LKG bits #74435

Closed
wants to merge 7 commits into from
Closed

AOT ILC using a LKG bits #74435

wants to merge 7 commits into from

Conversation

LakshanF
Copy link
Member

First draft of using a native AOT ILCompiler and builds on PR #73987 by importing the AOT target file, which will trigger publishing an AOT version of ILC.

The target, RewriteRuntimePackDir, is not used as is done by the crossgen2 package with this change. That will trigger using the runtime packages as done by crossgen2 but want to get a read of the LKG changes first.

@LakshanF
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member

Why do we need to use live ILCompiler targets if we want to use the LKG build? Why not just set PublishAot instead of what we have there now (SingleFile+Trimmed+ReadyToRun)?

@LakshanF LakshanF marked this pull request as ready for review August 31, 2022 16:21
@LakshanF
Copy link
Member Author

Why do we need to use live ILCompiler targets if we want to use the LKG build? Why not just set PublishAot instead of what we have there now (SingleFile+Trimmed+ReadyToRun)?

Makes sense to first do AOT with the LKG build.

<NativeAotSupported Condition="'$(DotNetBuildFromSource)' == 'true' or '$(TargetOS)' == 'osx'">false</NativeAotSupported>
<PublishAot Condition="'$(NativeAotSupported)' == 'true'">true</PublishAot>
<SelfContained>false</SelfContained>
<SelfContained Condition="'$(RunningPublish)' == 'true'">true</SelfContained>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the SelfContained properties necessary? I don't think we set RunningPublish on this project anywhere.

@MichalStrehovsky
Copy link
Member

Makes sense to first do AOT with the LKG build.

It was the title of this pull request :).

I think all of these changes should still go into the <!-- BEGIN: Workaround for https://github.com/dotnet/runtime/issues/67742 --> section (and the comment should not be deleted).

Using LKG is still just a workaround for #67742. The real fix would be doing the RunningPublish and all the other stuff that is necessary to use live bits. We would not be publishing into $(RuntimeBinDir)ilc-published as part of the clr partition build either because we don't have the libs to do that. It's all just workarounds.

@MichalStrehovsky
Copy link
Member

Thinking about this more - what do we get from this exercise? We needed the R2R trimmed singlefile stopgap because we were about to ship 7.0 and the ILCompiler nuget package was huge and slow to start. The stopgap got us halfway there.

Now we have more time to do the real fix (put things on the same plan as crossgen2 where we produce the bits with the live compiler in the installer partition build). What do we expect to gain from this temporary switchover?

@LakshanF
Copy link
Member Author

Thinking about this more - what do we get from this exercise? We needed the R2R trimmed singlefile stopgap because we were about to ship 7.0 and the ILCompiler nuget package was huge and slow to start. The stopgap got us halfway there.

Now we have more time to do the real fix (put things on the same plan as crossgen2 where we produce the bits with the live compiler in the installer partition build). What do we expect to gain from this temporary switchover?

Good point! I've also been thinking that its better to do the full fix and will close this PR

@LakshanF LakshanF closed this Sep 30, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 30, 2022
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.

3 participants