Skip to content

Commit

Permalink
Fix for IsVarAddr
Browse files Browse the repository at this point in the history
When I start using ILASM from VS environment instead of ILASM from the
CoreCLR build, I ran into an assertion for a test,
JIT\IL_Conformance\Old\directed\ldloca_s_r8:
https://github.com/dotnet/coreclr/blob/master/src/jit/codegenxarch.cpp#L7400-L7404
The MSILs from both ILASMs are technically identical, but only difference
is the presence of PDB file due to
https://github.com/dotnet/coreclr/issues/2982.
When PDB file is present, JIT ensures spilling individual il instructions
for good debugging experience.
As shown below, in the case of NoPDB, the type for the local address on
stack (from ldloca) is converted to an integer type since it is used for
type cast to double.
Note initially (when creating addr node) JIT starts from TY_BYREF
conservatively for such var address.
On the other hand in the case of PDB which failed to assert, JIT spills
this address node to a local var. By the time JIT imports conv.r8, JIT
uses the local var with TY_BYREF.
The existing "IsVarAddr" API didn't capture such case so it ended up with
feeding local var with byref type to cast operation, which is unexpected.

There may be several ways to address this issue.
The way I fix is to introduce a flag to local var (I can't simply add a
flag to tree-node whose bits run out so conflict with other use).
When JIT has to spill such var address (on stack) to a local var, JIT
records such bit to the local var.
I extends the implementation of ```IsVarAddr``` to
```impIsVarAddrOnStack``` in order to catch this case as well.
So, when JIT needs to hammer the type like using "impBashVarAddrsToI", JIT
now uses this API, instead.

Without PDB (pass)
```
[0] ldloca.s 0
[1] dup
[2] conv.r8

*  stmtExpr  void  (top level) (IL 0x000...  ???)
|  /--*  cast      double <- long                --> cast from long to double
|  |  \--*  addr      long                       --> This is local adress on stack, so type is hammered to long
|  |     \--*  lclVar    double V00 loc0
 \--*  =         double
 ```

With PDB (fail)
```
[0] ldloca.s 0

*  stmtExpr  void  (top level) (IL 0x000...  ???)
|  /--*  addr      byref
 |  |  \--*  lclVar    double V00 loc0
 \--*  =         byref
 \--*  lclVar    byref  V256 tmp0                 --> Address is spilled to local var.

[1] dup

*  stmtExpr  void  (top level) (IL 0x002...  ???)
\--*  no_op     void

[ 2] conv.r8

*  stmtExpr  void  (top level) (IL 0x003...  ???)
|  /--*  cast      double <- byref               --> !Assertion: Can't convert byref to double where the src is lclVar.
|  |  \--*  lclVar    byref  V256 tmp0
\--*  =         double
```

With PDB after fix (pass)
```
[0] ldloca.s 0

*  stmtExpr  void  (top level) (IL 0x000...  ???)
|  /--*  addr      byref
 |  |  \--*  lclVar    double V00 loc0
 \--*  =         byref
 \--*  lclVar    byref  V256 tmp0                 --> Address is spilled to local var. JIT sets a bit to it.

[1] dup

*  stmtExpr  void  (top level) (IL 0x002...  ???)
\--*  no_op     void

[ 2] conv.r8

*  stmtExpr  void  (top level) (IL 0x003...  ???)
|  /--*  cast      double <- long              --> byref is hammered to long same as the case of noPDB.
|  |  \--*  lclVar    long   V256 tmp0
\--*  =         double
```
  • Loading branch information
kyulee1 committed Mar 4, 2016
1 parent 4707152 commit 9f52bcd
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 36 deletions.
8 changes: 6 additions & 2 deletions src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ class LclVarDsc
unsigned char lvUsedInSIMDIntrinsic :1; // This tells lclvar is used for simd intrinsic
#endif // FEATURE_SIMD
unsigned char lvRegStruct :1; // This is a reg-sized non-field-addressed struct.
unsigned char lvIsVarAddrOnStack : 1; // This contains var address on stack.

union
{
Expand Down Expand Up @@ -2673,8 +2674,11 @@ protected :
CORINFO_FIELD_INFO * pFieldInfo,
var_types lclTyp);

static void impBashVarAddrsToI (GenTreePtr tree1,
GenTreePtr tree2 = NULL);
bool impBashVarAddrsToI(GenTreePtr tree1,
bool checkOnly = false);

void impBashVarAddrsToI(GenTreePtr tree1,
GenTreePtr tree2);

GenTreePtr impImplicitIorI4Cast(GenTreePtr tree,
var_types dstTyp);
Expand Down
16 changes: 0 additions & 16 deletions src/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1502,22 +1502,6 @@ void GenTree::ChangeOperUnchecked(genTreeOps oper)
gtFlags &= GTF_COMMON_MASK;
}


/*****************************************************************************
* Returns true if the node is &var (created by ldarga and ldloca)
*/

inline
bool GenTree::IsVarAddr() const
{
if (gtOper == GT_ADDR && (gtFlags & GTF_ADDR_ONSTACK))
{
assert((gtType == TYP_BYREF) || (gtType == TYP_I_IMPL));
return true;
}
return false;
}

/*****************************************************************************
*
* Returns true if the node is of the "ovf" variety, for example, add.ovf.i1.
Expand Down
79 changes: 63 additions & 16 deletions src/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1954,6 +1954,14 @@ bool Compiler::impSpillStackEntry(unsigned level,
// The tree type may be modified by impAssignTempGen, so use the type of the lclVar.
var_types type = genActualType(lvaTable[tnum].TypeGet());
GenTreePtr temp = gtNewLclvNode(tnum, type);

// If the tree contains var address on stack, propagate such flag to a local which is spilled.
// We do not update type for the local yet.
if (impBashVarAddrsToI(tree, true /* checkOnly */))
{
lvaTable[tnum].lvIsVarAddrOnStack = true;
}

verCurrentState.esStack[level].val = temp;

return true;
Expand Down Expand Up @@ -2559,21 +2567,63 @@ CORINFO_CLASS_HANDLE Compiler::impGetObjectClass()
return objectClass;
}

/*****************************************************************************
* Returns true if the node is &var (created by ldarga and ldloca)
* or local var that contains such stack address.
* If checkOnly is false, the type for tree and/or local is updated with TYP_I_IMPL
*/

bool Compiler::impBashVarAddrsToI(GenTreePtr tree, bool checkOnly)
{
if (tree == nullptr)
return false;

bool result = false;
#define INVALID_LOCAL_NUM -1
int localNum = INVALID_LOCAL_NUM;

if (tree->gtOper == GT_ADDR && (tree->gtFlags & GTF_ADDR_ONSTACK) != 0)
{
result = true;
}
else if (tree->gtOper == GT_LCL_VAR)
{
localNum = tree->AsLclVarCommon()->GetLclNum();
if (lvaTable[localNum].lvIsVarAddrOnStack)
{
result = true;
}
}

if (result)
{
assert((tree->gtType == TYP_BYREF) || (tree->gtType == TYP_I_IMPL));

if (!checkOnly)
{
tree->gtType = TYP_I_IMPL;
if (tree->gtOper == GT_LCL_VAR)
{
assert(localNum != INVALID_LOCAL_NUM);
lvaTable[localNum].lvType = TYP_I_IMPL;
}
}
}

return result;
}

/*****************************************************************************
* "&var" can be used either as TYP_BYREF or TYP_I_IMPL, but we
* set its type to TYP_BYREF when we create it. We know if it can be
* changed to TYP_I_IMPL only at the point where we use it
*/

/* static */
void Compiler::impBashVarAddrsToI(GenTreePtr tree1,
GenTreePtr tree2)
{
if ( tree1->IsVarAddr())
tree1->gtType = TYP_I_IMPL;

if (tree2 && tree2->IsVarAddr())
tree2->gtType = TYP_I_IMPL;
impBashVarAddrsToI(tree1);
impBashVarAddrsToI(tree2);
}

/*****************************************************************************
Expand Down Expand Up @@ -9074,7 +9124,7 @@ void Compiler::impImportBlockCode(BasicBlock * block)

// We had better assign it a value of the correct type
assertImp(genActualType(lclTyp) == genActualType(op1->gtType) ||
genActualType(lclTyp) == TYP_I_IMPL && op1->IsVarAddr() ||
genActualType(lclTyp) == TYP_I_IMPL && impBashVarAddrsToI(op1, true /* checkOnly */) ||
(genActualType(lclTyp) == TYP_I_IMPL && (op1->gtType == TYP_BYREF || op1->gtType == TYP_REF)) ||
(genActualType(op1->gtType) == TYP_I_IMPL && lclTyp == TYP_BYREF) ||
(varTypeIsFloating(lclTyp) && varTypeIsFloating(op1->TypeGet())) ||
Expand All @@ -9083,7 +9133,7 @@ void Compiler::impImportBlockCode(BasicBlock * block)
/* If op1 is "&var" then its type is the transient "*" and it can
be used either as TYP_BYREF or TYP_I_IMPL */

if (op1->IsVarAddr())
if (impBashVarAddrsToI(op1, true /* checkOnly */))
{
assertImp(genActualType(lclTyp) == TYP_I_IMPL || lclTyp == TYP_BYREF);

Expand Down Expand Up @@ -9866,8 +9916,7 @@ void Compiler::impImportBlockCode(BasicBlock * block)
op3 = impPopStack().val;

assertImp(op3->gtType == TYP_REF);
if (op2->IsVarAddr())
op2->gtType = TYP_I_IMPL;
impBashVarAddrsToI(op2);

op3 = impCheckForNullPointer(op3);

Expand Down Expand Up @@ -10129,7 +10178,7 @@ MATH_MAYBE_CALL_NO_OVF: ovfl = false;
}

op1 = impPopStack().val;
impBashVarAddrsToI(op1, NULL);
impBashVarAddrsToI(op1);
type = genActualType(op1->TypeGet());
impPushOnStack(gtNewOperNode(GT_NOT, type, op1), tiRetVal);
break;
Expand Down Expand Up @@ -10685,7 +10734,7 @@ MATH_MAYBE_CALL_NO_OVF: ovfl = false;
}

op1 = impPopStack().val;
impBashVarAddrsToI(op1, NULL);
impBashVarAddrsToI(op1);
impPushOnStack(gtNewOperNode(GT_NEG, genActualType(op1->gtType), op1), tiRetVal);
break;

Expand Down Expand Up @@ -16327,9 +16376,8 @@ void Compiler::impInlineInitVars(InlineInfo * pInlineInfo)
assert(sigType == TYP_I_IMPL);

/* If possible change the BYREF to an int */
if (thisArg->IsVarAddr())
if (impBashVarAddrsToI(thisArg))
{
thisArg->gtType = TYP_I_IMPL;
lclVarInfo[0].lclVerTypeInfo = typeInfo(varType2tiType(TYP_I_IMPL));
}
else
Expand Down Expand Up @@ -16410,9 +16458,8 @@ void Compiler::impInlineInitVars(InlineInfo * pInlineInfo)
assert(varTypeIsIntOrI(sigType));

/* If possible bash the BYREF to an int */
if (inlArgNode->IsVarAddr())
if (impBashVarAddrsToI(inlArgNode))
{
inlArgNode->gtType = TYP_I_IMPL;
lclVarInfo[i].lclVerTypeInfo = typeInfo(varType2tiType(TYP_I_IMPL));
}
else
Expand Down
3 changes: 1 addition & 2 deletions src/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2939,8 +2939,7 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
/* Change the node to TYP_I_IMPL so we don't report GC info
* NOTE: We deferred this from the importer because of the inliner */

if (argx->IsVarAddr())
argx->gtType = TYP_I_IMPL;
impBashVarAddrsToI(argx);

bool passUsingFloatRegs;
unsigned argAlign = 1;
Expand Down

0 comments on commit 9f52bcd

Please sign in to comment.