Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 78f53be

Browse files
erozenfelderozenfeld
authored
Merge pull request #24282 from erozenfeld/Fix24253
 Fix for a jit liveness bug.
2 parents 9cf8b4b + dc389a8 commit 78f53be

File tree

3 files changed

+147
-100
lines changed

3 files changed

+147
-100
lines changed

‎src/jit/liveness.cpp‎

Lines changed: 57 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -2235,137 +2235,95 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
22352235
return false;
22362236
}
22372237

2238-
/* Test for interior statement */
2238+
// Check for side effects
2239+
GenTree* sideEffList = nullptr;
2240+
if (rhsNode->gtFlags & GTF_SIDE_EFFECT)
2241+
{
2242+
#ifdef DEBUG
2243+
if (verbose)
2244+
{
2245+
printf(FMT_BB " - Dead assignment has side effects...\n", compCurBB->bbNum);
2246+
gtDispTree(asgNode);
2247+
printf("\n");
2248+
}
2249+
#endif // DEBUG
2250+
// Extract the side effects
2251+
if (rhsNode->TypeGet() == TYP_STRUCT)
2252+
{
2253+
// This is a block assignment. An indirection of the rhs is not considered to
2254+
// happen until the assignment, so we will extract the side effects from only
2255+
// the address.
2256+
if (rhsNode->OperIsIndir())
2257+
{
2258+
assert(rhsNode->OperGet() != GT_NULLCHECK);
2259+
rhsNode = rhsNode->AsIndir()->Addr();
2260+
}
2261+
}
2262+
gtExtractSideEffList(rhsNode, &sideEffList);
2263+
}
2264+
2265+
// Test for interior statement
22392266

22402267
if (asgNode->gtNext == nullptr)
22412268
{
2242-
/* This is a "NORMAL" statement with the
2243-
* assignment node hanging from the GT_STMT node */
2269+
// This is a "NORMAL" statement with the assignment node hanging from the GT_STMT node.
22442270

22452271
noway_assert(compCurStmt->gtStmtExpr == asgNode);
22462272
JITDUMP("top level assign\n");
22472273

2248-
/* Check for side effects */
2249-
2250-
if (rhsNode->gtFlags & GTF_SIDE_EFFECT)
2274+
if (sideEffList != nullptr)
22512275
{
2252-
EXTRACT_SIDE_EFFECTS:
2253-
/* Extract the side effects */
2254-
2255-
GenTree* sideEffList = nullptr;
2276+
noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT);
22562277
#ifdef DEBUG
22572278
if (verbose)
22582279
{
2259-
printf(FMT_BB " - Dead assignment has side effects...\n", compCurBB->bbNum);
2260-
gtDispTree(asgNode);
2280+
printf("Extracted side effects list...\n");
2281+
gtDispTree(sideEffList);
22612282
printf("\n");
22622283
}
22632284
#endif // DEBUG
2264-
if (rhsNode->TypeGet() == TYP_STRUCT)
2265-
{
2266-
// This is a block assignment. An indirection of the rhs is not considered to
2267-
// happen until the assignment, so we will extract the side effects from only
2268-
// the address.
2269-
if (rhsNode->OperIsIndir())
2270-
{
2271-
assert(rhsNode->OperGet() != GT_NULLCHECK);
2272-
rhsNode = rhsNode->AsIndir()->Addr();
2273-
}
2274-
}
2275-
gtExtractSideEffList(rhsNode, &sideEffList);
2276-
2277-
if (sideEffList)
2278-
{
2279-
noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT);
2280-
#ifdef DEBUG
2281-
if (verbose)
2282-
{
2283-
printf("Extracted side effects list...\n");
2284-
gtDispTree(sideEffList);
2285-
printf("\n");
2286-
}
2287-
#endif // DEBUG
22882285

2289-
/* Replace the assignment statement with the list of side effects */
2290-
noway_assert(sideEffList->gtOper != GT_STMT);
2286+
// Replace the assignment statement with the list of side effects
2287+
noway_assert(sideEffList->gtOper != GT_STMT);
22912288

2292-
*pTree = compCurStmt->gtStmtExpr = sideEffList;
2289+
*pTree = compCurStmt->gtStmtExpr = sideEffList;
22932290
#ifdef DEBUG
2294-
*treeModf = true;
2291+
*treeModf = true;
22952292
#endif // DEBUG
2296-
/* Update ordering, costs, FP levels, etc. */
2297-
gtSetStmtInfo(compCurStmt);
2298-
2299-
/* Re-link the nodes for this statement */
2300-
fgSetStmtSeq(compCurStmt);
2293+
// Update ordering, costs, FP levels, etc.
2294+
gtSetStmtInfo(compCurStmt);
23012295

2302-
// Since the whole statement gets replaced it is safe to
2303-
// re-thread and update order. No need to compute costs again.
2304-
*pStmtInfoDirty = false;
2296+
// Re-link the nodes for this statement
2297+
fgSetStmtSeq(compCurStmt);
23052298

2306-
/* Compute the live set for the new statement */
2307-
*doAgain = true;
2308-
return false;
2309-
}
2310-
else
2311-
{
2312-
/* No side effects, most likely we forgot to reset some flags */
2313-
fgRemoveStmt(compCurBB, compCurStmt);
2299+
// Since the whole statement gets replaced it is safe to
2300+
// re-thread and update order. No need to compute costs again.
2301+
*pStmtInfoDirty = false;
23142302

2315-
return true;
2316-
}
2303+
// Compute the live set for the new statement
2304+
*doAgain = true;
2305+
return false;
23172306
}
23182307
else
23192308
{
2320-
/* If this is GT_CATCH_ARG saved to a local var don't bother */
2321-
23222309
JITDUMP("removing stmt with no side effects\n");
23232310

2324-
if (asgNode->gtFlags & GTF_ORDER_SIDEEFF)
2325-
{
2326-
if (rhsNode->gtOper == GT_CATCH_ARG)
2327-
{
2328-
goto EXTRACT_SIDE_EFFECTS;
2329-
}
2330-
}
2331-
2332-
/* No side effects - remove the whole statement from the block->bbTreeList */
2333-
2311+
// No side effects - remove the whole statement from the block->bbTreeList
23342312
fgRemoveStmt(compCurBB, compCurStmt);
23352313

2336-
/* Since we removed it do not process the rest (i.e. RHS) of the statement
2337-
* variables in the RHS will not be marked as live, so we get the benefit of
2338-
* propagating dead variables up the chain */
2339-
2314+
// Since we removed it do not process the rest (i.e. RHS) of the statement
2315+
// variables in the RHS will not be marked as live, so we get the benefit of
2316+
// propagating dead variables up the chain
23402317
return true;
23412318
}
23422319
}
23432320
else
23442321
{
2345-
/* This is an INTERIOR STATEMENT with a dead assignment - remove it */
2346-
2322+
// This is an INTERIOR STATEMENT with a dead assignment - remove it
23472323
noway_assert(!VarSetOps::IsMember(this, life, varDsc->lvVarIndex));
23482324

2349-
if (rhsNode->gtFlags & GTF_SIDE_EFFECT)
2325+
if (sideEffList != nullptr)
23502326
{
2351-
/* :-( we have side effects */
2352-
2353-
GenTree* sideEffList = nullptr;
2354-
#ifdef DEBUG
2355-
if (verbose)
2356-
{
2357-
printf(FMT_BB " - INTERIOR dead assignment has side effects...\n", compCurBB->bbNum);
2358-
gtDispTree(asgNode);
2359-
printf("\n");
2360-
}
2361-
#endif // DEBUG
2362-
gtExtractSideEffList(rhsNode, &sideEffList);
2363-
2364-
if (!sideEffList)
2365-
{
2366-
goto NO_SIDE_EFFECTS;
2367-
}
2368-
23692327
noway_assert(sideEffList->gtFlags & GTF_SIDE_EFFECT);
23702328
#ifdef DEBUG
23712329
if (verbose)
@@ -2389,7 +2347,7 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
23892347
#ifdef DEBUG
23902348
*treeModf = true;
23912349
#endif // DEBUG
2392-
/* Change the node to a GT_COMMA holding the side effect list */
2350+
// Change the node to a GT_COMMA holding the side effect list
23932351
asgNode->gtBashToNOP();
23942352

23952353
asgNode->ChangeOper(GT_COMMA);
@@ -2409,7 +2367,6 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
24092367
}
24102368
else
24112369
{
2412-
NO_SIDE_EFFECTS:
24132370
#ifdef DEBUG
24142371
if (verbose)
24152372
{
@@ -2420,15 +2377,15 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
24202377
printf("\n");
24212378
}
24222379
#endif // DEBUG
2423-
/* No side effects - Change the assignment to a GT_NOP node */
2380+
// No side effects - Change the assignment to a GT_NOP node
24242381
asgNode->gtBashToNOP();
24252382

24262383
#ifdef DEBUG
24272384
*treeModf = true;
24282385
#endif // DEBUG
24292386
}
24302387

2431-
/* Re-link the nodes for this statement - Do not update ordering! */
2388+
// Re-link the nodes for this statement - Do not update ordering!
24322389

24332390
// Do not update costs by calling gtSetStmtInfo. fgSetStmtSeq modifies
24342391
// the tree threading based on the new costs. Removing nodes could
@@ -2439,7 +2396,7 @@ bool Compiler::fgRemoveDeadStore(GenTree** pTree,
24392396

24402397
fgSetStmtSeq(compCurStmt);
24412398

2442-
/* Continue analysis from this node */
2399+
// Continue analysis from this node
24432400

24442401
*pTree = asgNode;
24452402

‎tests/src/JIT/Regression/JitBlue/GitHub_24253/GitHub_24253.cs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
using System.Runtime.CompilerServices;
7+
8+
// Test removal of a dead struct assignment when the assignment is "internal"
9+
// i.e., is not a direct child of a statement node.
10+
// In this example, the statement is STMT(COMMA(CALL HELPER.CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE, ASG)).
11+
12+
namespace GitHub_24253
13+
{
14+
struct TestStruct
15+
{
16+
public static readonly TestStruct ZeroStruct = new TestStruct(0);
17+
18+
[MethodImpl(MethodImplOptions.NoInlining)]
19+
public TestStruct(int i)
20+
{
21+
this.i = i;
22+
this.j = 5;
23+
this.k = 5;
24+
this.l = 5;
25+
}
26+
27+
int i;
28+
int j;
29+
short k;
30+
short l;
31+
}
32+
33+
class Program
34+
{
35+
static int Main(string[] args)
36+
{
37+
GetStruct(1);
38+
return 100;
39+
}
40+
41+
[MethodImpl(MethodImplOptions.NoInlining)]
42+
private static TestStruct GetStruct(int i)
43+
{
44+
// This is the dead assignment that is causing the bug assert.
45+
TestStruct result = TestStruct.ZeroStruct;
46+
try
47+
{
48+
result = new TestStruct(i);
49+
}
50+
catch
51+
{
52+
throw new ArgumentException();
53+
}
54+
return result;
55+
}
56+
}
57+
}

‎tests/src/JIT/Regression/JitBlue/GitHub_24253/GitHub_24253.csproj‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
3+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
4+
<PropertyGroup>
5+
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
6+
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
7+
<SchemaVersion>2.0</SchemaVersion>
8+
<ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
9+
<OutputType>Exe</OutputType>
10+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
11+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
12+
</PropertyGroup>
13+
<!-- Default configurations to help VS understand the configurations -->
14+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
15+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' " />
16+
<ItemGroup>
17+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
18+
<Visible>False</Visible>
19+
</CodeAnalysisDependentAssemblyPaths>
20+
</ItemGroup>
21+
<PropertyGroup>
22+
<DebugType>None</DebugType>
23+
<Optimize>True</Optimize>
24+
</PropertyGroup>
25+
<ItemGroup>
26+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
27+
</ItemGroup>
28+
<ItemGroup>
29+
<Compile Include="$(MSBuildProjectName).cs" />
30+
</ItemGroup>
31+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
32+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
33+
</Project>

0 commit comments

Comments
 (0)