Skip to content

Commit

Permalink
Fix for asserting var addr on stack
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 the following assertion in codegenxarch.cpp.
```
noway_assert(op1->OperGet() == GT_LCL_VAR_ADDR || op1->OperGet() == GT_LCL_FLD_ADDR);
```
for the test, JIT\IL_Conformance\Old\directed\ldloca_s_r8.
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 each il instruction
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.

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), and
propagate a bit when such var address node is spilled.
The assertion is relaxed based on this bit for GT_LCL_VAR.
Preivously I attempted to aggressively hammer the type by refactoing
```impBashVarAddrsToI``` in various ways, but it was not straightforward
to handle other assertions that are popped up.

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

|  /--*  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
```
  • Loading branch information
kyulee1 committed Mar 4, 2016
1 parent 4707152 commit 979dea7
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 3 deletions.
9 changes: 6 additions & 3 deletions src/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7454,13 +7454,16 @@ CodeGen::genIntToFloatCast(GenTreePtr treeNode)

// Since xarch emitter doesn't handle reporting gc-info correctly while casting away gc-ness we
// ensure srcType of a cast is non gc-type. Codegen should never see BYREF as source type except
// for GT_LCL_VAR_ADDR and GT_LCL_FLD_ADDR that represent stack addresses and can be considered
// as TYP_I_IMPL. In all other cases where src operand is a gc-type and not known to be on stack,
// for GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR and GT_LCL_VAR with lvIsVarAddrOnStack
// that represent stack addresses and can be considered as TYP_I_IMPL.
// In all other cases where src operand is a gc-type and not known to be on stack,
// Front-end (see fgMorphCast()) ensures this by assigning gc-type local to a non gc-type
// temp and using temp as operand of cast operation.
if (srcType == TYP_BYREF)
{
noway_assert(op1->OperGet() == GT_LCL_VAR_ADDR || op1->OperGet() == GT_LCL_FLD_ADDR);
noway_assert(op1->OperGet() == GT_LCL_VAR_ADDR || op1->OperGet() == GT_LCL_FLD_ADDR
|| (op1->OperGet() == GT_LCL_VAR &&
!!compiler->lvaTable[op1->gtLclVarCommon.GetLclNum()].lvIsVarAddrOnStack));
srcType = TYP_I_IMPL;
}

Expand Down
1 change: 1 addition & 0 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
6 changes: 6 additions & 0 deletions src/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1951,6 +1951,12 @@ bool Compiler::impSpillStackEntry(unsigned level,
/* Assign the spilled entry to the temp */
impAssignTempGen(tnum, tree, verCurrentState.esStack[level].seTypeInfo.GetClassHandle(), level);

// If the tree contains var address on stack, propagate such flag to a local which is spilled.
if (tree->IsVarAddr())
{
lvaTable[tnum].lvIsVarAddrOnStack = true;
}

// 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);
Expand Down

0 comments on commit 979dea7

Please sign in to comment.