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 identical code folding on Linux #87045

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jun 2, 2023

This instructs the linker to deduplicate identical sections on Linux. We were previously not doing this for two reasons:

  1. We weren't passing the command line switch.
  2. We had trouble getting linker to actually fold things. Linux linkers are particularly picky on what they're willing to fold. We've been historically putting these things to RW sections (Move InterfaceDispatchMapTable to data section corert#686) and that doesn't help. Looking at C++, it looks like it places vtables into .text section, so let's just do the same.

I also found out why --foldmethodbodies doesn't work outside Windows - we place managed code into a __managedcode section and Linux linkers won't fold sections that have names that are valid C identifiers (would need to name this .managedcode like on Windows) - https://github.com/dotnet/llvm-project/blob/c01ca3bc8a420b3796d816c43bdd06e337c72ea6/lld/ELF/ICF.cpp#L192. Not addressing that part because the whole thing looks like a hairy yak and this option is not supported anyway.

Cc @dotnet/ilc-contrib

This instructs the linker to deduplicate identical sections on Linux. We were previously not doing this for two reasons:

1. We weren't passing the command line switch.
2. We had trouble getting linker to actually fold things. Linux linkers are particularly picky on what they're willing to fold. We've been historically putting these things to RW sections (dotnet/corert#686) and that doesn't help. Looking at C++, it looks like it places vtables into `.text` section, so let's just do the same.

I also found out why `--foldmethodbodies` doesn't work outside Windows - we place managed code into a `__managedcode` section and Linux linkers won't fold sections that have names that are valid C identifiers (would need to name this `.managedcode` like on Windows) - https://github.com/dotnet/llvm-project/blob/c01ca3bc8a420b3796d816c43bdd06e337c72ea6/lld/ELF/ICF.cpp#L192. Not addressing that part because the whole thing looks like a hairy yak and this option is not supported anyway.
@ghost
Copy link

ghost commented Jun 2, 2023

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

Issue Details

This instructs the linker to deduplicate identical sections on Linux. We were previously not doing this for two reasons:

  1. We weren't passing the command line switch.
  2. We had trouble getting linker to actually fold things. Linux linkers are particularly picky on what they're willing to fold. We've been historically putting these things to RW sections (Move InterfaceDispatchMapTable to data section corert#686) and that doesn't help. Looking at C++, it looks like it places vtables into .text section, so let's just do the same.

I also found out why --foldmethodbodies doesn't work outside Windows - we place managed code into a __managedcode section and Linux linkers won't fold sections that have names that are valid C identifiers (would need to name this .managedcode like on Windows) - https://github.com/dotnet/llvm-project/blob/c01ca3bc8a420b3796d816c43bdd06e337c72ea6/lld/ELF/ICF.cpp#L192. Not addressing that part because the whole thing looks like a hairy yak and this option is not supported anyway.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@ghost ghost assigned MichalStrehovsky Jun 2, 2023
@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Jun 2, 2023

After the second commit this is just an okay saving:

  LLD Baseline LLD PR
DynamicGenerics 5,059,776 5,036,560
HardwareIntrinsics 1,608,008 1,600,968

@@ -19,7 +19,7 @@ public override ObjectNodeSection GetSection(NodeFactory factory)
if (factory.Target.IsWindows)
Copy link
Member

Choose a reason for hiding this comment

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

Should we extract this piece of logic into a helper method on ObjectNodeSection or NodeFactory so that we do not need to fix up a bunch of places everytime a new right way to do foldable readonly data sections shows up?

@filipnavara
Copy link
Member

Can we run the NativeAOT extras pipeline for this? There's lot of quirks with ld64 on macOS and I am bit worried that the section rearrangement may trip up something.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -151,6 +151,7 @@ The .NET Foundation licenses this file to you under the MIT license.
<LinkerArg Include="-Wl,-segprot,__THUNKS,rx,rx" Condition="'$(_IsiOSLikePlatform)' == 'true'" />
<LinkerArg Include="@(NativeFramework->'-framework %(Identity)')" Condition="'$(_IsApplePlatform)' == 'true'" />
<LinkerArg Include="-Wl,--eh-frame-hdr" Condition="'$(_IsApplePlatform)' != 'true'" />
<LinkerArg Include="-Wl,--icf=all" Condition="'$(LinkerFlavor)' == 'lld' or '$(LinkerFlavor)' == 'gold'" />
Copy link
Member

@am11 am11 Jun 3, 2023

Choose a reason for hiding this comment

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

Suggested change
<LinkerArg Include="-Wl,--icf=all" Condition="'$(LinkerFlavor)' == 'lld' or '$(LinkerFlavor)' == 'gold'" />
<LinkerArg Include="-Wl,--icf=all" Condition="'$(_IsApplePlatform)' != 'true' and '$(LinkerFlavor)' != 'bfd'" />

as mold also supports it.

@am11 am11 mentioned this pull request Oct 3, 2023
7 tasks
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Feb 5, 2024
Extracting a piece of dotnet#87045 that I had to revert in that PR. Native linkers don't like when LSDA is in a COMDAT so fold these in the object writer instead. Seems to save about 1.2% in the Stage1 app. Obviously Unix only.
MichalStrehovsky added a commit that referenced this pull request Feb 7, 2024
Extracting a piece of #87045 that I had to revert in that PR. Native linkers don't like when LSDA is in a COMDAT so fold these in the object writer instead. Seems to save about 1.2% in the Stage1 app. Obviously Unix only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants