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

Switch to selfhosted NativeAOT compiler #81205

Merged
merged 8 commits into from
Mar 9, 2023

Conversation

MichalStrehovsky
Copy link
Member

This will compile the NativeAOT compiler with the LKG build of the NativeAOT compiler that the repo is building with.

At this point NativeAOT-compiled compiler is significantly better than the R2R+Trimmed+SingleFile-compiled combo we're shipping right now.

Time to compile hello world before: 2.3 seconds. Time to compile hello world after: 1.3 seconds.
ilc.exe size before: 31 MB. ilc.exe size after: 14 MB.

Cc @dotnet/ilc-contrib

@ghost
Copy link

ghost commented Jan 26, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This will compile the NativeAOT compiler with the LKG build of the NativeAOT compiler that the repo is building with.

At this point NativeAOT-compiled compiler is significantly better than the R2R+Trimmed+SingleFile-compiled combo we're shipping right now.

Time to compile hello world before: 2.3 seconds. Time to compile hello world after: 1.3 seconds.
ilc.exe size before: 31 MB. ilc.exe size after: 14 MB.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@@ -10,6 +10,7 @@
<!-- BEGIN: Workaround for https://github.com/dotnet/runtime/issues/67742 -->
<PropertyGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'">
<PublishDir>$(RuntimeBinDir)ilc-published/</PublishDir>
<NativeAotSupported Condition="'$(RuntimeIdentifier)' != 'win-x64' and '$(RuntimeIdentifier)' != 'linux-x64'">false</NativeAotSupported>
Copy link
Member

Choose a reason for hiding this comment

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

Why exclude macOS (and arm64)?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the previous failed attempts: Directory.Build.targets(83,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) The passed-in TargetOS property value 'OSX' must be lowercase.

Directory.Build.targets became opinionated about how to spell OSX. The old ilc probably doesn't spell it right. I don't particularly care about osx in the scope of this PR - this is to speed up testing and we do the majority of testing on Linux and Windows.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can broaden this pretty easily after we get it started and flush out the problems.

agocke
agocke previously approved these changes Jan 31, 2023
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM. An eventual improvement will be moving this to a test dependency so we don't have to rely on BuildingInsideVisualStudio

@@ -10,6 +10,7 @@
<!-- BEGIN: Workaround for https://github.com/dotnet/runtime/issues/67742 -->
<PropertyGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'">
<PublishDir>$(RuntimeBinDir)ilc-published/</PublishDir>
<NativeAotSupported Condition="'$(RuntimeIdentifier)' != 'win-x64' and '$(RuntimeIdentifier)' != 'linux-x64'">false</NativeAotSupported>
Copy link
Member

Choose a reason for hiding this comment

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

I think we can broaden this pretty easily after we get it started and flush out the problems.

<PublishSingleFile>true</PublishSingleFile>
<NativeAotSupported Condition="'$(RuntimeIdentifier)' != 'win-x64' and '$(RuntimeIdentifier)' != 'linux-x64'">false</NativeAotSupported>
<PublishAot Condition="'$(NativeAotSupported)' == 'true'">true</PublishAot>
<PublishReadyToRun Condition="'$(NativeAotSupported)' != 'true'">true</PublishReadyToRun>
Copy link
Member

Choose a reason for hiding this comment

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

Consider moving this to a separate ItemGroup with a top-level NativeAOTSupported condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already in a conditioned itemgroup so I would need to repeat the condition. Given this is eventually going away in favor of a live build, maybe it's fine.

@agocke agocke dismissed their stale review January 31, 2023 19:40

test failures

@agocke
Copy link
Member

agocke commented Jan 31, 2023

Ah, just looked at the test failures. Looks like the configuration is not correct somewhere -- we're passing --target incorrectly for both gcc and freebsd.

@MichalStrehovsky
Copy link
Member Author

@am11 FYI - using gcc with PublishAot seems broken for crossbuild. I tried working around in this PR, but that ran into the issue that this is not using the live Microsoft.NETCore.Native.Unix.targets and my workaround is a noop.

I'm going to meditate on this some more because I don't know what the workaround for the gcc legs would be (I guess we could fix the freebsd one by checking TargetOS).

@am11
Copy link
Member

am11 commented Feb 14, 2023

@am11 FYI - using gcc with PublishAot seems broken for crossbuild.

Yes, it is a known issue. Cross compilation doesn't work with gcc #78559. Unlike clang, gcc is arch-specific and doesn't support -target arg.

This will compile the NativeAOT compiler with the LKG build of the NativeAOT compiler that the repo is building with.

At this point NativeAOT-compiled compiler is significantly better than the R2R+Trimmed+SingleFile-compiled combo we're shipping right now.

ilc.exe size before: 31 MB. ilc.exe size after: 14 MB.
Time to compile hello world before: 2.3 seconds. Time to compile hello world after: 1.3 seconds.
@MichalStrehovsky
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 Author

@dotnet/ilc-contrib This is ready for review.

<!-- Don't R2R on ARM64 machines because ARM64 crossgen2 that comes with .NET SDK <= 7.0 Preview 7 crashes.-->
<PublishReadyToRun Condition="'$(BuildArchitecture)' != 'arm64'">true</PublishReadyToRun>
<PublishSingleFile>true</PublishSingleFile>
<NativeAotSupported Condition="'$(TargetOS)' != 'windows' and '$(TargetOS)' != 'linux'">false</NativeAotSupported>
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

I can't comment on the viability of osx, but the rest of the change looks good.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky merged commit 434d664 into dotnet:main Mar 9, 2023
@MichalStrehovsky MichalStrehovsky deleted the selfhostilc branch March 9, 2023 23:30
@MichalStrehovsky
Copy link
Member Author

I can't comment on the viability of osx, but the rest of the change looks good.

Looks like the osx issue was also fixed in the latest SDK drop so I enabled it.

@eerhardt
Copy link
Member

We see a nice drop in our benchmarks "Build Time", likely due to this change:

image

@MichalStrehovsky
Copy link
Member Author

We see a nice drop in our benchmarks "Build Time", likely due to this change:

Went better than expected!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2023
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.

6 participants