Linker leaves dead code for substituted methods

[tannergooding]

The linker will perform some small constant folding for substituted methods or static readonly T fields. However, it leaves various "crumbs" behind that can quickly add up in a larger library.

Consider for example, the relatively simple scenario here: https://github.com/terrafx/terrafx.interop.windows/pull/341

While the substitution does cause TerraFX.Interop.Configuration.DisableResolveLibraryHook to be evaluated to a constant true, it's use site leaves behind a dead compare+branch, which in turn leaves behind a static constructor that could otherwise be removed:

static Windows()
{
  if (true)
    ;
}

Similar "crumbs" can be seen throughout System.Private.Corelib for things like System.IntPtr.Size, various Isa.IsSupported checks for hardware intrinsics, and more. This represents unnecessary "bloat" that is kept in the trimmed output and makes the JIT and other tools have to work harder to remove these dead blocks.

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

Issue Details

---

The linker will perform some small constant folding for substituted methods or `static readonly T` fields. However, it leaves various "crumbs" behind that can quickly add up in a larger library. Consider for example, the relatively simple scenario here: https://github.com/terrafx/terrafx.interop.windows/pull/341 While the substitution does cause `TerraFX.Interop.Configuration.DisableResolveLibraryHook` to be evaluated to a `constant true`, it's use site leaves behind a dead `compare+branch`, which in turn leaves behind a `static constructor` that could otherwise be removed: ```csharp static Windows() { if (true) ; } ``` Similar "crumbs" can be seen throughout `System.Private.Corelib` for things like `System.IntPtr.Size`, various `Isa.IsSupported` checks for hardware intrinsics, and more. This represents unnecessary "bloat" that is kept in the trimmed output and makes the JIT and other tools have to work harder to remove these dead blocks.

Author:	tannergooding
Assignees:	-
Labels:	`untriaged`, `area-Tools-ILLink`
Milestone:	-

[tannergooding]

I had done some initial digging into this and it looks like the general handling for this is in UnreachableBlocksOptimizer.cs

From what I could tell, we effectively fill in a FoldedInstructions list withnop representing logic that can be evaluated as constant. We then use that to build a reachableInstrs map, and then attempt to remove the unreachable code.

We however don't actually go back and remove or otherwise reprocess the logic that was initially determined to be "foldable" and so they are left in "as is" which can lead to "crumbs" in the form of field reads, empty if (true) { } blocks, and similar.

Is that about right and then would the fix be to ensure that the FoldedInstructions list is processed much as the trailing logic processes the out var nopInstructions created via bodySweeper.Produce()?

If so, could we change bodySweeper.Produce to mark things in FoldedInstructions instead of producing its own "new list" and then just removing anything marked nop from FoldedInstructions instead?

[vitek-karas]

Adding @marek-safar as the original author of this logic. I would have to dig into this in detail to answer the questions... didn't have time to do so yet.

[marek-safar]

The logic would be more complex than just removing a few nops. In general, we are missing a step (logic) in ILLink that would optimize method IL and that code would be better done as an extra step than when processing unreachable sections.