Skip to content

Commit

Permalink
Take into account zero-offset field sequences when propagating locals (
Browse files Browse the repository at this point in the history
…#64701)

* Add a test

* Fix the problem

LCL_VAR use VN != VN of its SSA def.

Using one for replacement can run afoul of substituting
into a LCL_VAR that has a zero-offset field sequence on
top and thus duplicate the sequence (as the replacement
def will already have this sequence incorporated into its
VN).

* Fix the same problem in local propagation

Just one diff, missing null check elimination: we propagated a field
sequence for an array address (PtrToArrElem) that later wasn't recognized
as implying non-nullness of another PtrToArrElem to the same array.
  • Loading branch information
SingleAccretion authored Feb 8, 2022
1 parent 75589ae commit 713c8dd
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 22 deletions.
67 changes: 52 additions & 15 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,11 @@ void Compiler::optPrintAssertion(AssertionDsc* curAssertion, AssertionIndex asse
{
printf(".%02u", curAssertion->op1.lcl.ssaNum);
}
if (curAssertion->op2.zeroOffsetFieldSeq != nullptr)
{
printf(" Zero");
gtDispFieldSeq(curAssertion->op2.zeroOffsetFieldSeq);
}
break;

case O2K_CONST_INT:
Expand Down Expand Up @@ -1582,10 +1587,14 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1,
goto DONE_ASSERTION; // Don't make an assertion
}

assertion.op2.kind = O2K_LCLVAR_COPY;
assertion.op2.lcl.lclNum = lclNum2;
assertion.op2.vn = vnStore->VNConservativeNormalValue(op2->gtVNPair);
assertion.op2.lcl.ssaNum = op2->AsLclVarCommon()->GetSsaNum();
FieldSeqNode* zeroOffsetFieldSeq = nullptr;
GetZeroOffsetFieldMap()->Lookup(op2, &zeroOffsetFieldSeq);

assertion.op2.kind = O2K_LCLVAR_COPY;
assertion.op2.vn = vnStore->VNConservativeNormalValue(op2->gtVNPair);
assertion.op2.lcl.lclNum = lclNum2;
assertion.op2.lcl.ssaNum = op2->AsLclVarCommon()->GetSsaNum();
assertion.op2.zeroOffsetFieldSeq = zeroOffsetFieldSeq;

// Ok everything has been set and the assertion looks good
assertion.assertionKind = assertionKind;
Expand Down Expand Up @@ -3316,14 +3325,30 @@ GenTree* Compiler::optCopyAssertionProp(AssertionDsc* curAssertion,
return nullptr;
}

// Extract the matching lclNum and ssaNum.
const unsigned copyLclNum = (op1.lcl.lclNum == lclNum) ? op2.lcl.lclNum : op1.lcl.lclNum;
unsigned copySsaNum = SsaConfig::RESERVED_SSA_NUM;
// Extract the matching lclNum and ssaNum, as well as the field sequence.
unsigned copyLclNum;
unsigned copySsaNum;
FieldSeqNode* zeroOffsetFieldSeq;
if (op1.lcl.lclNum == lclNum)
{
copyLclNum = op2.lcl.lclNum;
copySsaNum = op2.lcl.ssaNum;
zeroOffsetFieldSeq = op2.zeroOffsetFieldSeq;
}
else
{
copyLclNum = op1.lcl.lclNum;
copySsaNum = op1.lcl.ssaNum;
zeroOffsetFieldSeq = nullptr; // Only the RHS of an assignment can have a FldSeq.
assert(optLocalAssertionProp); // Were we to perform replacements in global propagation, that makes copy
// assertions for control flow ("if (a == b) { ... }"), where both operands
// could have a FldSeq, we'd need to save it for "op1" too.
}

if (!optLocalAssertionProp)
{
// Extract the ssaNum of the matching lclNum.
unsigned ssaNum = (op1.lcl.lclNum == lclNum) ? op1.lcl.ssaNum : op2.lcl.ssaNum;
copySsaNum = (op1.lcl.lclNum == lclNum) ? op2.lcl.ssaNum : op1.lcl.ssaNum;

if (ssaNum != tree->GetSsaNum())
{
Expand All @@ -3349,12 +3374,25 @@ GenTree* Compiler::optCopyAssertionProp(AssertionDsc* curAssertion,
tree->SetLclNum(copyLclNum);
tree->SetSsaNum(copySsaNum);

// The sequence we are propagating (if any) represents the inner fields.
if (zeroOffsetFieldSeq != nullptr)
{
FieldSeqNode* outerZeroOffsetFieldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(tree, &outerZeroOffsetFieldSeq))
{
zeroOffsetFieldSeq = GetFieldSeqStore()->Append(zeroOffsetFieldSeq, outerZeroOffsetFieldSeq);
GetZeroOffsetFieldMap()->Remove(tree);
}

fgAddFieldSeqForZeroOffset(tree, zeroOffsetFieldSeq);
}

#ifdef DEBUG
if (verbose)
{
printf("\nAssertion prop in " FMT_BB ":\n", compCurBB->bbNum);
optPrintAssertion(curAssertion, index);
gtDispTree(tree, nullptr, nullptr, true);
DISPNODE(tree);
}
#endif

Expand Down Expand Up @@ -4665,15 +4703,15 @@ GenTree* Compiler::optAssertionProp(ASSERT_VALARG_TP assertions, GenTree* tree,
}

//------------------------------------------------------------------------
// optImpliedAssertions: Given a tree node that makes an assertion this
// method computes the set of implied assertions
// that are also true. The updated assertions are
// maintained on the Compiler object.
// optImpliedAssertions: Given an assertion this method computes the set
// of implied assertions that are also true.
//
// Arguments:
// assertionIndex : The id of the assertion.
// activeAssertions : The assertions that are already true at this point.

// This method will add the discovered implied assertions
// to this set.
//
void Compiler::optImpliedAssertions(AssertionIndex assertionIndex, ASSERT_TP& activeAssertions)
{
noway_assert(!optLocalAssertionProp);
Expand Down Expand Up @@ -4822,7 +4860,6 @@ void Compiler::optImpliedByTypeOfAssertions(ASSERT_TP& activeAssertions)
// Return Value:
// The assertions we have about the value number.
//

ASSERT_VALRET_TP Compiler::optGetVnMappedAssertions(ValueNum vn)
{
ASSERT_TP set = BitVecOps::UninitVal();
Expand Down
13 changes: 9 additions & 4 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -7627,7 +7627,6 @@ class Compiler
O2K_CONST_INT,
O2K_CONST_LONG,
O2K_CONST_DOUBLE,
O2K_ARR_LEN,
O2K_SUBRANGE,
O2K_COUNT
};
Expand Down Expand Up @@ -7667,7 +7666,11 @@ class Compiler
GenTreeFlags iconFlags; // gtFlags
};
union {
SsaVar lcl;
struct
{
SsaVar lcl;
FieldSeqNode* zeroOffsetFieldSeq;
};
IntVal u1;
__int64 lconVal;
double dconVal;
Expand Down Expand Up @@ -7746,6 +7749,7 @@ class Compiler
{
return false;
}

switch (op2.kind)
{
case O2K_IND_CNS_INT:
Expand All @@ -7760,9 +7764,9 @@ class Compiler
return (memcmp(&op2.dconVal, &that->op2.dconVal, sizeof(double)) == 0);

case O2K_LCLVAR_COPY:
case O2K_ARR_LEN:
return (op2.lcl.lclNum == that->op2.lcl.lclNum) &&
(!vnBased || op2.lcl.ssaNum == that->op2.lcl.ssaNum);
(!vnBased || op2.lcl.ssaNum == that->op2.lcl.ssaNum) &&
(op2.zeroOffsetFieldSeq == that->op2.zeroOffsetFieldSeq);

case O2K_SUBRANGE:
return op2.u2.Equals(that->op2.u2);
Expand All @@ -7775,6 +7779,7 @@ class Compiler
assert(!"Unexpected value for op2.kind in AssertionDsc.");
break;
}

return false;
}

Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/jit/copyprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ void Compiler::optCopyProp(Statement* stmt,
continue;
}

if (newLclDefVN != tree->gtVNPair.GetConservative())
ValueNum lclDefVN = varDsc->GetPerSsaData(tree->GetSsaNum())->m_vnPair.GetConservative();
if (newLclDefVN != lclDefVN)
{
continue;
}
Expand Down Expand Up @@ -230,9 +231,9 @@ void Compiler::optCopyProp(Statement* stmt,
{
JITDUMP("VN based copy assertion for ");
printTreeID(tree);
printf(" V%02d " FMT_VN " by ", lclNum, tree->GetVN(VNK_Conservative));
printf(" V%02d " FMT_VN " by ", lclNum, lclDefVN);
printTreeID(newLclDefNode);
printf(" V%02d " FMT_VN ".\n", newLclNum, newLclDefNode->GetVN(VNK_Conservative));
printf(" V%02d " FMT_VN ".\n", newLclNum, newLclDefVN);
DISPNODE(tree);
}
#endif
Expand Down
82 changes: 82 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_64700/Runtime_64700.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Numerics;
using System.Runtime.CompilerServices;

class Runtime_64700
{
private static StructWithVtors _structWithVtorsStatic;

static int Main()
{
_structWithVtorsStatic = new StructWithVtors { StructWithOneVtor = { OneVtor = new Vector2(1, 0) } };

if (ProblemWithCopyProp(0) != 0)
{
return 101;
}

if (ProblemWithLocalAssertionProp(new SmallerStruct[] { default }) != 1)
{
return 102;
}

return 100;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static float ProblemWithCopyProp(float a)
{
ref var p1 = ref _structWithVtorsStatic.StructWithOneVtor.OneVtor;
ref var p2 = ref _structWithVtorsStatic;

if (_structWithVtorsStatic.StructWithOneVtor.OneVtor.X == 1)
{
p2.StructWithOneVtor.OneVtor = Vector2.Zero;
if (_structWithVtorsStatic.StructWithOneVtor.OneVtor.X == 1)
{
a = 1;
}
}

return a + p1.Y;
}

struct StructWithVtors
{
public StructWithOneVtor StructWithOneVtor;
}

struct StructWithOneVtor
{
public Vector2 OneVtor;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static long ProblemWithLocalAssertionProp(SmallerStruct[] a)
{
ref var p1 = ref a[0];
Use(ref p1);
ref var p2 = ref p1.RegStruct;
Use(ref p2);

var t = p2.FirstLngValue;
a[0].RegStruct.FirstLngValue = 1;

return t + p2.FirstLngValue;
}

public static void Use<T>(ref T arg) { }

struct SmallerStruct
{
public RegStruct RegStruct;
}

struct RegStruct
{
public long FirstLngValue;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
<CLRTestBatchPreCommands><![CDATA[
$(CLRTestBatchPreCommands)
set DOTNET_JitOptRepeat=ProblemWithCopyProp
]]></CLRTestBatchPreCommands>
<BashCLRTestPreCommands><![CDATA[
$(BashCLRTestPreCommands)
export DOTNET_JitOptRepeat=ProblemWithCopyProp
]]></BashCLRTestPreCommands>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 713c8dd

Please sign in to comment.