Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Bug fixes from dead code elimination by kunalspathak · Pull Request #69897 · dotnet/runtime · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 32 additions & 4 deletions src/coreclr/jit/fgopt.cpp
Original file line numberDiff line numberDiff 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 DownExpand Up@@ -713,16 +714,43 @@ 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);

#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
isRemovable &=
!block->isBBCallAlwaysPairTail(); // can't remove the BBJ_ALWAYS of a BBJ_CALLFINALLY / BBJ_ALWAYS pair
#endif
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 +760,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 numberDiff line numberDiff 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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 numberDiff line numberDiff 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 numberDiff line numberDiff 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 numberDiff line numberDiff 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>