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

Commit e80e040

Browse files
erozenfelderozenfeld
committed
Fix for #21011: propagate GTF_DONT_CSE on comma returns
When a method returns a multi-reg struct, we set GTF_DONT_CSE on return local: https://github.com/dotnet/coreclr/blob/497419bf8f19c649d821295da7e225e55581cce9/src/jit/importer.cpp#L8783 Setting GTF_DONT_CSE blocks assertion propagation here: https://github.com/dotnet/coreclr/blob/9d49bf1ec6f102b89e5c2885e8f9d3d77f2ec144/src/jit/assertionprop.cpp#L2845-L2848 In the test we have a synchronized method so we change the return node to return a comma that include a call to HELPER.CORINFO_HELP_MON_EXIT. If the rightmost comma expression doesn't have GTF_DONT_CSE, assertion propagation is not blocked and we end up with this tree: ``` [000040] -----+------ /--* CNS_INT struct 0 [000043] --C-G+------ /--* COMMA struct [000036] --C-G+------ | \--* CALL help void HELPER.CORINFO_HELP_MON_EXIT [000032] L----+------ arg1 in x1 | +--* ADDR long [000031] ----G+-N---- | | \--* LCL_VAR ubyte (AX) V03 tmp1 [000033] -----+------ arg0 in x0 | \--* LCL_VAR ref V00 this [000041] -AC-G+------ * COMMA struct [000006] -----+-N---- | /--* LCL_VAR struct V01 loc0 [000039] -A---+------ \--* ASG struct (copy) [000037] D----+-N---- \--* LCL_VAR struct V05 tmp3 ``` Downstream phases can't handle struct zero return expressed as ``` [000040] -----+------ /--* CNS_INT struct 0 ``` The fix is to propagate GTF_DONT_CSE to the rightmost comma expression to block bad assertion propagation. Fixes #21011.
1 parent 382874f commit e80e040

File tree

4 files changed

+118
-1
lines changed

4 files changed

+118
-1
lines changed

‎format.patch‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
2+
index ad1fd83fe9..bcc818f25b 100644
3+
--- a/src/jit/flowgraph.cpp
4+
+++ b/src/jit/flowgraph.cpp
5+
@@ -8016,45 +8016,46 @@ GenTree* Compiler::fgCreateMonitorTree(unsigned lvaMonAcquired, unsigned lvaThis
6+
{
7+
GenTree* retNode = block->lastStmt()->gtStmtExpr;
8+
GenTree* retExpr = retNode->gtOp.gtOp1;
9+
10+
if (retExpr != nullptr)
11+
{
12+
// have to insert this immediately before the GT_RETURN so we transform:
13+
// ret(...) ->
14+
// ret(comma(comma(tmp=...,call mon_exit), tmp)
15+
//
16+
//
17+
// Before morph stage, it is possible to have a case of GT_RETURN(TYP_LONG, op1) where op1's type is
18+
// TYP_STRUCT (of 8-bytes) and op1 is call node. See the big comment block in impReturnInstruction()
19+
// for details for the case where info.compRetType is not the same as info.compRetNativeType. For
20+
// this reason pass compMethodInfo->args.retTypeClass which is guaranteed to be a valid class handle
21+
// if the return type is a value class. Note that fgInsertCommFormTemp() in turn uses this class handle
22+
// if the type of op1 is TYP_STRUCT to perform lvaSetStruct() on the new temp that is created, which
23+
// in turn passes it to VM to know the size of value type.
24+
GenTree* temp = fgInsertCommaFormTemp(&retNode->gtOp.gtOp1, info.compMethodInfo->args.retTypeClass);
25+
26+
- GenTree* lclVar = retNode->gtOp.gtOp1->gtOp.gtOp2;
27+
+ GenTree* lclVar = retNode->gtOp.gtOp1->gtOp.gtOp2;
28+
29+
// The return can't handle all of the trees that could be on the right-hand-side of an assignment,
30+
// especially in the case of a struct. Therefore, we need to propagate GTF_DONT_CSE.
31+
- // If we don't, assertion propagation may, e.g., change a return of a local to a return of "CNS_INT struct 0",
32+
+ // If we don't, assertion propagation may, e.g., change a return of a local to a return of "CNS_INT struct
33+
+ // 0",
34+
// which downstream phases can't handle.
35+
lclVar->gtFlags |= (retExpr->gtFlags & GTF_DONT_CSE);
36+
retNode->gtOp.gtOp1->gtOp.gtOp2 = gtNewOperNode(GT_COMMA, retExpr->TypeGet(), tree, lclVar);
37+
}
38+
else
39+
{
40+
// Insert this immediately before the GT_RETURN
41+
fgInsertStmtNearEnd(block, tree);
42+
}
43+
}
44+
else
45+
{
46+
fgInsertStmtAtEnd(block, tree);
47+
}
48+
49+
return tree;
50+
}
51+
52+
// Convert a BBJ_RETURN block in a synchronized method to a BBJ_ALWAYS.
53+
// We've previously added a 'try' block around the original program code using fgAddSyncMethodEnterExit().

‎src/jit/flowgraph.cpp‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8033,7 +8033,14 @@ GenTree* Compiler::fgCreateMonitorTree(unsigned lvaMonAcquired, unsigned lvaThis
80338033
// in turn passes it to VM to know the size of value type.
80348034
GenTree* temp = fgInsertCommaFormTemp(&retNode->gtOp.gtOp1, info.compMethodInfo->args.retTypeClass);
80358035

8036-
GenTree* lclVar = retNode->gtOp.gtOp1->gtOp.gtOp2;
8036+
GenTree* lclVar = retNode->gtOp.gtOp1->gtOp.gtOp2;
8037+
8038+
// The return can't handle all of the trees that could be on the right-hand-side of an assignment,
8039+
// especially in the case of a struct. Therefore, we need to propagate GTF_DONT_CSE.
8040+
// If we don't, assertion propagation may, e.g., change a return of a local to a return of "CNS_INT struct
8041+
// 0",
8042+
// which downstream phases can't handle.
8043+
lclVar->gtFlags |= (retExpr->gtFlags & GTF_DONT_CSE);
80378044
retNode->gtOp.gtOp1->gtOp.gtOp2 = gtNewOperNode(GT_COMMA, retExpr->TypeGet(), tree, lclVar);
80388045
}
80398046
else

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
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+
using System.Collections.Generic;
8+
9+
public class Test
10+
{
11+
public static int Main()
12+
{
13+
Test test = new Test();
14+
test.GetPair();
15+
return 100;
16+
}
17+
18+
[MethodImpl(MethodImplOptions.Synchronized | MethodImplOptions.NoInlining)]
19+
internal KeyValuePair<uint, float>? GetPair()
20+
{
21+
KeyValuePair<uint,float>? result = new KeyValuePair<uint,float>?();
22+
return result;
23+
}
24+
}

‎tests/src/JIT/Regression/JitBlue/Github_21011/Github_21011.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>{2649FAFE-07BF-4F93-8120-BA9A69285ABB}</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' "></PropertyGroup>
16+
<PropertyGroup>
17+
<DebugType>None</DebugType>
18+
<Optimize>True</Optimize>
19+
</PropertyGroup>
20+
<ItemGroup>
21+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
22+
<Visible>False</Visible>
23+
</CodeAnalysisDependentAssemblyPaths>
24+
</ItemGroup>
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)