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

[wasm] Add the illink substitutions for SIMD #79672

Merged

Conversation

radekdoulik
Copy link
Member

@radekdoulik radekdoulik commented Dec 14, 2022

This should fix size regression introduced with #78068

The untrimmed S.N.Vector class added few kilobytes to the assemblies of the bench sample. That wasn't that bad, OTOH the code produced by the aot compiler was much larger, 5MB in this case.

So this will case default non-SIMD case. In the case of SIMD build we will need more investigation.

This should fix size regression introduce with dotnet#78068

The untrimmed S.N.Vector class added few kilobytes to the assemblies
of the bench sample. That wasn't that bad, OTOH the code produced
by the aot compiler was much larger, 5MB in this case.

So this will case default non-SIMD case. In the case of SIMD build we
will need more investigation.
@radical
Copy link
Member

radical commented Dec 14, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +112 to +113
<_ExtraTrimmerArgs Condition="'$(WasmEnableSIMD)' == 'true'">$(_ExtraTrimmerArgs) --substitutions $(MSBuildThisFileDirectory)/ILLink.Substitutions.WasmIntrinsics.xml</_ExtraTrimmerArgs>
<_ExtraTrimmerArgs Condition="'$(WasmEnableSIMD)' != 'true'">$(_ExtraTrimmerArgs) --substitutions $(MSBuildThisFileDirectory)/ILLink.Substitutions.NoWasmIntrinsics.xml</_ExtraTrimmerArgs>
Copy link
Member

Choose a reason for hiding this comment

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

This is using private properties from the ILLink sdk. Since this file is shipped, we need to use an alternate way to pass these arguments.

Copy link
Member

Choose a reason for hiding this comment

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

This property is suggested in the repo docs: https://github.com/dotnet/linker/blob/2db841fc5f6bc7592b66ffe3bd0e1c888d022f93/docs/illink-options.md?plain=1#L3-L7

The illink is IL linker version shipping with .NET Core or .NET 5 platforms. It's bundled with the .NET SDK and most of the options are accessible using msbuild properties but any option can also be passed using _ExtraTrimmerArgs property.

And it is being used by multiple other projects like xamarin-macios.
@marek-safar Is there an alternate property that should be used for passing arbitrary arguments to the linker?

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked @vitek-karas about it yesterday and he thinks there's currently no other way to set that.

Copy link
Member

Choose a reason for hiding this comment

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

Adding @sbomer who knows best.

If this is an important scenario we could add more official support for it in the linker SDK bits.

@ghost
Copy link

ghost commented Dec 14, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

This should fix size regression introduced with #78068

The untrimmed S.N.Vector class added few kilobytes to the assemblies of the bench sample. That wasn't that bad, OTOH the code produced by the aot compiler was much larger, 5MB in this case.

So this will case default non-SIMD case. In the case of SIMD build we will need more investigation.

Author: radekdoulik
Assignees: radekdoulik
Labels:

arch-wasm, area-Build-mono

Milestone: -

@radical
Copy link
Member

radical commented Dec 14, 2022

The AOT test failures are #79569, and unrelated.

@radical radical merged commit a788114 into dotnet:main Dec 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants