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

Enable x86 AOT compilation for components build #105909

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Aug 3, 2024

Now that global.json is pointing to p6 (which has an SDK fix for win-x86), lets try enabling it.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 3, 2024
Copy link
Contributor

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

@am11
Copy link
Member Author

am11 commented Aug 3, 2024

...
  Generating native code
  ILCompiler -> D:\a\_work\1\s\artifacts\bin\coreclr\windows.x86.Checked\ilc-published\
...

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

This change looks good.

I think there is a separate question whether we should be building win-x86 ilc and crossgen2 binaries at all. I have tried to compile hello world using win-x86 ilc from .NET 9 P6 SDK and the build insists on cross-compiling using win-x64 ilc ("Add a PackageReference for 'runtime.win-x64.Microsoft.DotNet.ILCompiler' to allow cross-compilation for x86"). cc @filipnavara

@filipnavara
Copy link
Member

I have tried to compile hello world using win-x86 ilc from .NET 9 P6 SDK and the build insists on cross-compiling using win-x64 ilc ("Add a PackageReference for 'runtime.win-x64.Microsoft.DotNet.ILCompiler' to allow cross-compilation for x86").

That’s somewhat unexpected. It was possible to use for building the tests last time I tried. It was not necessarily a good experience since it was prone to run out of virtual memory on any bigger inputs.

@jkotas
Copy link
Member

jkotas commented Aug 3, 2024

We use custom setup for tests, so I can believe that it is possible to use it for tests. I was looking at it from the user point of view. As far as I can tell, there is no way for regular user project files to use the win-x86 ilc binary.

@am11
Copy link
Member Author

am11 commented Aug 3, 2024

@jkotas, second commit fixes the publishing issue you described; when dotnet.exe or ilc.exe is x86, running (emulating) on x64 or arm64 machine. x86 dotnet.exe or ilc.exe on x86 windows were working fine with p6, so it wasn't widespread.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thanks!

@MichalStrehovsky
Copy link
Member

@agocke, are you okay with merging this? I noticed your #105826 and this takes a dependency on preview 6. I don't have context on the other PR.

@agocke
Copy link
Member

agocke commented Aug 5, 2024

There were a couple issues popping up that seemed to be caused by low frequency bugs in the preview 6 SDK. I was evaluating if we could roll it back, while monitoring the build. Overall, I don’t think we should roll back. The issues most commonly blocking the build don’t seem to be ones that would be fixed by a rollback, and we’re close to p7. I think it’s fine to merge this.

@MichalStrehovsky MichalStrehovsky merged commit 283d5f3 into dotnet:main Aug 5, 2024
144 of 149 checks passed
@MichalStrehovsky
Copy link
Member

There were a couple issues popping up that seemed to be caused by low frequency bugs in the preview 6 SDK. I was evaluating if we could roll it back, while monitoring the build. Overall, I don’t think we should roll back. The issues most commonly blocking the build don’t seem to be ones that would be fixed by a rollback, and we’re close to p7. I think it’s fine to merge this.

Thank you! Merging!

@am11 am11 deleted the patch-9 branch August 5, 2024 17:47
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x86 area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants