Optimization removes necessary code

[shibaev]

Description

A NativeAOT version of our project fails at runtime while the related managed version works. There are not publishing warnings and static analysis warnings.

The root cause is that the publishing removes some required code. The code is simple without reflection or any similar dynamic techniques.

Reproduction Steps

Program.cs

namespace AotIssue
{
    internal class Program
    {
        static void Main(string[] args)
        {
            string s = "12151";
            int i = 0;

            char? a = null;
            while (true)
            {
                string? res = get(s, ref i, ref a);
                if (res != null)
                {
                    Console.Write(res);
                    a = null; // !!! this line is removed from the published version

                    // Console.Write(a); // uncomment the line for the workaround
                    continue;
                }

                if (i >= s.Length)
                    break;

                a = '.';
            }
        }

        static string? get(string s, ref int index, ref char? a)
        {
            if (index >= s.Length)
                return null;

            if (a == '.')
                return ".";

            a ??= s[index++];
            return (a == '1') ? "1" : null;
        }
    }
}

AotIssue.csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishAot>true</PublishAot>
    <InvariantGlobalization>true</InvariantGlobalization>
    <IsAotCompatible>true</IsAotCompatible>
  </PropertyGroup>
</Project>

To publish the project, you can use the dotnet publish -r win-x64 -c Release command. Or similar publish profile:

<?xml version="1.0" encoding="utf-8"?>
<!--
https://go.microsoft.com/fwlink/?LinkID=208121.
-->
<Project>
  <PropertyGroup>
    <Configuration>Release</Configuration>
    <Platform>Any CPU</Platform>
    <PublishDir>bin\Release\net8.0\publish\win-x64\</PublishDir>
    <PublishProtocol>FileSystem</PublishProtocol>
    <_TargetId>Folder</_TargetId>
    <TargetFramework>net8.0</TargetFramework>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
    <SelfContained>true</SelfContained>
    <PublishSingleFile>false</PublishSingleFile>
    <PublishReadyToRun>false</PublishReadyToRun>
  </PropertyGroup>
</Project>

Expected behavior

The AOT version prints "1.1.1" like regular .NET version.

Actual behavior

The AOT version prints an infinite sequence of "1111111111111...". The loop never ends.

Regression?

No response

Known Workarounds

Uncomment the line Console.Write(a);. In this case, the code works as expected after publishing.

Configuration

I reproduced the issue in the .NET 8 project with win-x64 target platform on Windows 10.

Other information

The line a = null; is removed from the published version.

[am11]

Repros with 9.0.100-preview.6.24313.50 as well (osx-arm64).

[dotnet-policy-service[bot]]

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

[dotnet-policy-service[bot]]

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

[jkotas]

This repros with DOTNET_TieredCompilation=0 with a JIT as well.

[EgorBo]

Looks to be optRemoveRedundantZeroInits issue

[jakobbotsch]

Looks to be optRemoveRedundantZeroInits issue

More specifically, BB02 is not marked with BBF_BACKWARD_JUMP that optRemoveRedundantZeroInits is relying on to check whether some zero inits are necessary. Tail merging managed to get rid of the a = null inside the loop by rewriting the loop as follows:

string s = "12151";
int i = 0;

char? a;
while (true)
{
    a = null;
    AfterNulling:;
    string? res = get(s, ref i, ref a);
    if (res != null)
    {
        Console.Write(res);
        continue;
    }

    if (i >= s.Length)
        break;

    a = '.';
    goto AfterNulling;
}

The a = null at the beginning of the loop is in a block that tail merging created but did not mark with BBF_BACKWARD_JUMP, and then optRemoveRedundantZeroInits removes it.

I wonder if we can just reuse some of our flow graph analysis to make this determination during optRemoveRedundantZeroInits. We have both the DFS tree and loops available at this point, though we still need to consider whether the block is part of any irreducible cycles. It seems very fragile to try to rely on something like BBF_BACKWARD_JUMP this late.

cc @AndyAyersMS

[EgorBo]

@jakobbotsch Yes, I also diagnosed lack of BBF_BACKWARD_JUMP yesterday due to tail merge - I didn't put up a fix because I was not sure how to fix it in place in fgTailMerge (around fgRedirectTargetEdge call) since I couldn't just rely on, potentially, stale bbNum data. I was wondering if fgRenumberBlocks should just set this flag

[EgorBo]

and looks like we can give up on BBF_BACKWARD_JUMP_SOURCE and BBF_BACKWARD_JUMP_TARGET since they're only used in importer. Overall I am surprised we've never hit this issue before - we do quite a few BB manipulations but we never set BBF_BACKWARD_JUMP anywhere after importer, we only set it in various block splits.

[AndyAyersMS]

Yes this is too fragile. We should audit the other uses of BBF_BACKWARD_JUMP and make sure there aren't more that might cause problems.

Seems like we could just stop walking blocks when we hit a block that has a DFS backedge? It might miss a few cases we get now, but seems easier to understand.

[AndyAyersMS]

Have a fix for this implemented for main, should have a PR up soon. It will need to be expressed differently in 8.0.