From f6eded26022145c80bf6309e59313a85d8a09f8d Mon Sep 17 00:00:00 2001 From: Pat Gavlin Date: Fri, 11 Nov 2016 10:20:59 -0800 Subject: [PATCH] Admit more dest addresses in block assign value-numbering. Internal testing revealed an assertion when computing the value number for block assignments: the VN framework expected any block assignments to a tracked lclVar to have a destination address of the form `(addr (lclVar))` or `(addr (lclFld))`. However, the check that it uses to determine whether or not a block assignment targets a lclVar also admits addresses formed by some combination of adds of constants to these patterns (e.g. `(add (const 4) (add (addr lclVar) (const 4)))`. This fix admits these additional patterns in value numbering by allowing `ExtendPtrVN(GenTree*, GenTree*)` to supply `NotAField` to `ExtendPtrVN(GenTree*, FieldSeqNode*)`. --- src/jit/valuenum.cpp | 9 +- .../JitBlue/DevDiv_278523/DevDiv_278523.il | 113 +++++++++++++++++++++ .../JitBlue/DevDiv_278523/DevDiv_278523.ilproj | 41 ++++++++ 3 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.il create mode 100644 tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.ilproj diff --git a/src/jit/valuenum.cpp b/src/jit/valuenum.cpp index c9bb277..51937da 100644 --- a/src/jit/valuenum.cpp +++ b/src/jit/valuenum.cpp @@ -2656,7 +2656,7 @@ ValueNum ValueNumStore::ExtendPtrVN(GenTreePtr opA, GenTreePtr opB) if (opB->OperGet() == GT_CNS_INT) { FieldSeqNode* fldSeq = opB->gtIntCon.gtFieldSeq; - if ((fldSeq != nullptr) && (fldSeq != FieldSeqStore::NotAField())) + if (fldSeq != nullptr) { return ExtendPtrVN(opA, opB->gtIntCon.gtFieldSeq); } @@ -2666,8 +2666,9 @@ ValueNum ValueNumStore::ExtendPtrVN(GenTreePtr opA, GenTreePtr opB) ValueNum ValueNumStore::ExtendPtrVN(GenTreePtr opA, FieldSeqNode* fldSeq) { + assert(fldSeq != nullptr); + ValueNum res = NoVN; - assert(fldSeq != FieldSeqStore::NotAField()); ValueNum opAvnWx = opA->gtVNPair.GetLiberal(); assert(VNIsValid(opAvnWx)); @@ -4982,17 +4983,21 @@ void Compiler::fgValueNumberBlockAssignment(GenTreePtr tree, bool evalAsgLhsInd) assert(lhs->OperGet() == GT_IND); lhsAddr = lhs->gtOp.gtOp1; } + // For addr-of-local expressions, lib/cons shouldn't matter. assert(lhsAddr->gtVNPair.BothEqual()); ValueNum lhsAddrVN = lhsAddr->GetVN(VNK_Liberal); // Unpack the PtrToLoc value number of the address. assert(vnStore->IsVNFunc(lhsAddrVN)); + VNFuncApp lhsAddrFuncApp; vnStore->GetVNFunc(lhsAddrVN, &lhsAddrFuncApp); + assert(lhsAddrFuncApp.m_func == VNF_PtrToLoc); assert(vnStore->IsVNConstant(lhsAddrFuncApp.m_args[0]) && vnStore->ConstantValue(lhsAddrFuncApp.m_args[0]) == lhsLclNum); + lhsFldSeq = vnStore->FieldSeqVNToFieldSeq(lhsAddrFuncApp.m_args[1]); } diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.il b/tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.il new file mode 100644 index 0000000..9c9ba49 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.il @@ -0,0 +1,113 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +.assembly extern mscorlib {} +.assembly a {} +.module a.exe + +// This test originally triggered an assert when computing the value number for a block assignment. In particular, the +// VN framework expected any block assignments to a tracked lclVar to have a destination address of the form +// `(addr (lclVar))` or `(addr (lclFld))`. The check that it was using to determine whether or not a block assignment +// targets a lclVar also admitted addresses formed by some combination of adds of constants to these patterns (e.g. +// `(add (const 4) (add (addr lclVar) (const 4)))`. The bits of IL that trigger the assert are called out in the method +// bodies below. They differ for 32- and 64-bit targets because on 64-bit targets, the importer will insert an +// int->long conversion when adding a constant int to any long. Due to the cast, the resulting IR is not considered to +// be an add of a constant and a lclVar address. In order to repro the bug on a 64-bit target, the input IL must +// directly produce a long constant. + +.class private sequential ansi sealed beforefieldinit S extends [mscorlib]System.ValueType +{ + .field public uint8 m_fld + .field public uint8 m_fld1 + .field public uint8 m_fld2 + .field public uint8 m_fld3 + .field public uint8 m_fld4 + .field public uint8 m_fld5 + .field public uint8 m_fld6 +} + +.class private sequential ansi sealed beforefieldinit T extends [mscorlib]System.ValueType +{ + .field public int32 m_int + .field public valuetype S m_fld +} + +.class private abstract auto ansi sealed beforefieldinit C extends [mscorlib]System.Object +{ + .method private static int32 Test32Bit(int32 i) noinlining + { + .locals init (valuetype S V_0, valuetype T V_1) + + ldloca.s V_0 + ldarg.0 + conv.u1 + stfld uint8 S::m_fld6 + + // This sequence of IL repros the issue. + ldloca.s V_1 + ldc.i4.4 + add + ldloc.0 + stobj S + + ldloca.s V_1 + ldfld valuetype S T::m_fld + ldfld uint8 S::m_fld6 + conv.i4 + ret + } + + .method private static int32 Test64Bit(int32 i) noinlining + { + .locals init (valuetype S V_0, valuetype T V_1) + + ldloca.s V_0 + ldarg.0 + conv.u1 + stfld uint8 S::m_fld6 + + // This sequence of IL repros the issue. Note that the `ldc.i8` is necessary (rather than an `ldc.i4` that is + // implicitly converted to a long byt the `add`). + ldloca.s V_1 + ldc.i8 4 + add + ldloc.0 + stobj S + + ldloca.s V_1 + ldfld valuetype S T::m_fld + ldfld uint8 S::m_fld6 + conv.i4 + ret + } + + .method private static int32 Main() + { + .entrypoint + .locals init (int32 V_0) + + ldc.i4 100 + dup + + sizeof [mscorlib]System.IntPtr + ldc.i4 8 + beq.s _64bit + + call int32 C::Test32Bit(int32) + bne.un.s fail + br.s success + +_64bit: + call int32 C::Test64Bit(int32) + bne.un.s fail + +success: + ldc.i4 100 + ret + +fail: + ldc.i4 101 + ret + } +} diff --git a/tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.ilproj b/tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.ilproj new file mode 100644 index 0000000..d4cfe45 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/DevDiv_278523/DevDiv_278523.ilproj @@ -0,0 +1,41 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT .0\UITestExtensionPackages + ..\..\ + 7a9bfb7d + + + + + + + + + False + + + + None + True + + + + + + + + + + + -- 2.7.4