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

Bug fixes from dead code elimination #69897

Merged
merged 8 commits into from
Jun 6, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ bool Compiler::fgRemoveDeadBlocks()
JITDUMP("\n*************** In fgRemoveDeadBlocks()");

jitstd::list<BasicBlock*> worklist(jitstd::allocator<void>(getAllocator(CMK_Reachability)));

worklist.push_back(fgFirstBB);

// Do not remove handler blocks
Expand Down Expand Up @@ -713,16 +714,39 @@ bool Compiler::fgRemoveDeadBlocks()
}
}

// Track if there is any unreachable block. Even if it is marked with
// BBF_DONT_REMOVE, fgRemoveUnreachableBlocks() still removes the code
// inside the block. So this variable tracks if we ever found such blocks
// or not.
bool hasUnreachableBlock = false;

// A block is unreachable if no path was found from
// any of the fgFirstBB, handler, filter or BBJ_ALWAYS (Arm) blocks.
auto isBlockRemovable = [&](BasicBlock* block) -> bool {
return !BlockSetOps::IsMember(this, visitedBlocks, block->bbNum);
bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum);
bool isRemovable = (!isVisited || block->bbRefs == 0);

hasUnreachableBlock |= isRemovable;

return isRemovable;
};

bool changed = fgRemoveUnreachableBlocks(isBlockRemovable);
bool changed = false;
unsigned iterationCount = 1;
do
{
JITDUMP("\nRemoving unreachable blocks for fgRemoveDeadBlocks iteration #%u\n", iterationCount);

// Just to be paranoid, avoid infinite loops; fall back to minopts.
if (iterationCount++ > 10)
{
noway_assert(!"Too many unreachable block removal loops");
}
changed = fgRemoveUnreachableBlocks(isBlockRemovable);
} while (changed);

#ifdef DEBUG
if (verbose && changed)
if (verbose && hasUnreachableBlock)
{
printf("\nAfter dead block removal:\n");
fgDispBasicBlocks(verboseTrees);
Expand All @@ -732,7 +756,7 @@ bool Compiler::fgRemoveDeadBlocks()
fgVerifyHandlerTab();
fgDebugCheckBBlist(false);
#endif // DEBUG
return changed;
return hasUnreachableBlock;
}

//-------------------------------------------------------------
Expand Down
35 changes: 35 additions & 0 deletions src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_1.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
//
// In this issue, although we were not removing an unreachable block, we were removing all the code
// inside it and as such should update the liveness information. Since we were not updating the liveness
// information for such scenarios, we were hitting an assert during register allocation.
public class Program
{
public static ulong[] s_14;
public static uint s_34;
public static int Main()
{
var vr2 = new ulong[][]{new ulong[]{0}};
M27(s_34, vr2);
return 100;
}

public static void M27(uint arg4, ulong[][] arg5)
{
arg5[0][0] = arg5[0][0];
for (int var7 = 0; var7 < 1; var7++)
{
return;
}

try
{
s_14 = arg5[0];
}
finally
{
arg4 = arg4;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late review, but GitHub_XYZ refers to the dotnet/coreclr repo, so this should have been named Runtime_69659. Also it would be nice to name the class something like Runtime_69659, it makes it easier to track the test down on failures.

It seems there are a few tests incorrectly filed under GitHub_XYZ tag, so maybe something to consider cleaning up as part of a future change.

Copy link
Member Author

Choose a reason for hiding this comment

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

so this should have been named Runtime_69659

Ah, didn't realize that. I named them _1 and _2 because there were 2 separate issues that were filed in #69659. I think I forgot to have the right class name in one of those files.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, Runtime_69569_1 etc. sounds right then.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<PropertyGroup>
<DebugType>PdbOnly</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
46 changes: 46 additions & 0 deletions src/tests/JIT/Regression/JitBlue/GitHub_69659/GitHub_69659_2.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
//
// In this issue, we were not removing all the unreachable blocks and that led us to expect that
// there should be an IG label for one of the unreachable block, but we were not creating it leading
// to an assert failure.
public class _65659_2
{
public static bool[][,] s_2;
public static short[,][] s_8;
public static bool[] s_10;
public static ushort[][] s_29;
public static int Main()
{
bool vr1 = M47();
return 100;
}

public static bool M47()
{
bool var0 = default(bool);
try
{
if (var0)
{
try
{
if (s_10[0])
{
return s_2[0][0, 0];
}
}
finally
{
s_8[0, 0][0] = 0;
}
}
}
finally
{
s_29 = s_29;
}

return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<PropertyGroup>
<DebugType>PdbOnly</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>