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

Commit

Permalink
Jit: fix a few issues with single def local tracking (#10633)
Browse files Browse the repository at this point in the history
Fixes for three issues that came up in desktop testing.

1. Don't track class types for locals when in import only mode,
since the jit may be looking at an uninstantiated generic method.
In these cases the jit's use of TYP_REF to represent CORINFO_TYPE_VAR
confuses the tracking code. In import only mode the jit is not going
to use the information, so there is no need to track it at all.

    Import only mode is tied to verification and CoreCLR runs in full
trust mode, so no test was added.

2. Allow `lvaUpdateClass` to be called more than once but assert
that the second and subsequent calls provide the exact same update
as the first call.

    This can happen for single def ref class locals whose definition
block is reimported. Reimportation is triggered by some tolerable
non-ref type mismatches at block joins and so ref type information
should be consistent for each importation attempt.

    Add a couple of IL test cases translated from destkop MC++ tests
that will trigger this kind of reimportation.

3. Bail out of devirtualization (for desktop only) if the base class
has precise initialization semantics, since desktop seems to trigger
class initialization for ref type callvirts (contrary to ECMA355 I.8.9.5).
See #4853 for some discussion. Since this is desktop only behavior
no test was added.
  • Loading branch information
AndyAyersMS committed Apr 1, 2017
1 parent 7c8941a commit a7ab04c
Show file tree
Hide file tree
Showing 7 changed files with 1,008 additions and 3 deletions.
8 changes: 8 additions & 0 deletions src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16379,6 +16379,14 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTreePtr tree, bool* isExact,
*isExact = false;
CORINFO_CLASS_HANDLE objClass = nullptr;

// Bail out if we're just importing and not generating code, since
// the jit uses TYP_REF for CORINFO_TYPE_VAR locals and args, but
// these may not be ref types.
if (compIsForImportOnly())
{
return objClass;
}

// Bail out if the tree is not a ref type.
var_types treeType = tree->TypeGet();
if (treeType != TYP_REF)
Expand Down
12 changes: 12 additions & 0 deletions src/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18488,6 +18488,18 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
CORINFO_CLASS_HANDLE baseClass = info.compCompHnd->getMethodClass(baseMethod);
const DWORD baseClassAttribs = info.compCompHnd->getClassAttribs(baseClass);

#if !defined(FEATURE_CORECLR)
// If base class is not beforefieldinit then devirtualizing may
// cause us to miss a base class init trigger. Spec says we don't
// need a trigger for ref class callvirts but desktop seems to
// have one anyways. So defer.
if ((baseClassAttribs & CORINFO_FLG_BEFOREFIELDINIT) == 0)
{
JITDUMP("\nimpDevirtualizeCall: base class has precise initialization, sorry\n");
return;
}
#endif // FEATURE_CORECLR

// Is the call an interface call?
const bool isInterface = (baseClassAttribs & CORINFO_FLG_INTERFACE) != 0;

Expand Down
34 changes: 31 additions & 3 deletions src/jit/lclvars.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2306,6 +2306,15 @@ void Compiler::lvaSetStruct(unsigned varNum, CORINFO_CLASS_HANDLE typeHnd, bool
void Compiler::lvaSetClass(unsigned varNum, CORINFO_CLASS_HANDLE clsHnd, bool isExact)
{
noway_assert(varNum < lvaCount);

// If we are just importing, we cannot reliably track local ref types,
// since the jit maps CORINFO_TYPE_VAR to TYP_REF.
if (compIsForImportOnly())
{
return;
}

// Else we should have a type handle.
assert(clsHnd != nullptr);

LclVarDsc* varDsc = &lvaTable[varNum];
Expand Down Expand Up @@ -2378,6 +2387,15 @@ void Compiler::lvaSetClass(unsigned varNum, GenTreePtr tree, CORINFO_CLASS_HANDL
void Compiler::lvaUpdateClass(unsigned varNum, CORINFO_CLASS_HANDLE clsHnd, bool isExact)
{
noway_assert(varNum < lvaCount);

// If we are just importing, we cannot reliably track local ref types,
// since the jit maps CORINFO_TYPE_VAR to TYP_REF.
if (compIsForImportOnly())
{
return;
}

// Else we should have a class handle to consider
assert(clsHnd != nullptr);

LclVarDsc* varDsc = &lvaTable[varNum];
Expand All @@ -2386,12 +2404,22 @@ void Compiler::lvaUpdateClass(unsigned varNum, CORINFO_CLASS_HANDLE clsHnd, bool
// We should already have a class
assert(varDsc->lvClassHnd != nullptr);

// This should be the first and only update for this var
assert(!varDsc->lvClassInfoUpdated);

#if defined(DEBUG)

// In general we only expect one update per local var. However if
// a block is re-imported and that block has the only STLOC for
// the var, we may see multiple updates. All subsequent updates
// should agree on the type, since reimportation is triggered by
// type mismatches for things other than ref types.
if (varDsc->lvClassInfoUpdated)
{
assert(varDsc->lvClassHnd == clsHnd);
assert(varDsc->lvClassIsExact == isExact);
}

// This counts as an update, even if nothing changes.
varDsc->lvClassInfoUpdated = true;

#endif // defined(DEBUG)

// If previous type was exact, there is nothing to update. Would
Expand Down
Loading

0 comments on commit a7ab04c

Please sign in to comment.