Skip to content

Commit

Permalink
[interp] add guard that a local hasn't been modified for stlocfld fus…
Browse files Browse the repository at this point in the history 
…ion (mono/mono#18129)

[interp] add guard that a local hasn't been modified for stlocfld fusion

Fixes mono/mono#18120



Commit migrated from mono/mono@e7b98d3
  • Loading branch information
@lewurm @monojenkins
lewurm authored and monojenkins committed Dec 19, 2019
1 parent 9420382 commit 53d4500
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 16 deletions.
91 changes: 91 additions & 0 deletions src/mono/mono/mini/iltests.il
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3358,6 +3358,97 @@ wrong:
ret
}

.class nested private auto ansi beforefieldinit R1_class
{
.field public int32 test_field

.method public hidebysig specialname rtspecialname instance default void '.ctor' () cil managed
{
ret
}
}

.method public hidebysig static int32 test_3_ldstloc_opt () cil managed
{
.locals init (
class Tests/R1_class v_0,
int32 v_1,
class Tests/R1_class v_2
)

/* allocate first object */
newobj instance void class Tests/R1_class::.ctor ()
stloc.0
ldloc.0
stloc.2

br L3
/* dead code for interp opt barrier */
ldc.i4.1
ldc.i4.1
add
pop
L3:
/* load first object on the stack */
ldloc.0

ldc.i4.3
stloc.1
ldloc.1

/* allocate second object */
newobj instance void class Tests/R1_class::.ctor ()
/* overwrite first object at loc0 with second object */
stloc.0

/* should store '3' into first object.
* in the buggy case it stores '3' into the second object because it delays the load from loc.0 */
stfld int32 Tests/R1_class::test_field

/* load first object on the stack */
ldloc.2
/* should read '3' */
ldfld int32 Tests/R1_class::test_field
ret
}

.method public hidebysig static void ldstarg_opt_helper (Tests/R1_class a) cil managed
{
ldarg.0

/* allocate second object */
newobj instance void class Tests/R1_class::.ctor ()
dup
ldc.i4.1
/* stores '1' in the field */
stfld int32 Tests/R1_class::test_field
/* overwrite first argument */
starg 0

ldc.i4.3
/* stores '3' in the field */
stfld int32 Tests/R1_class::test_field /* might get fused with ldarg.0 at start of method */
ret
}

.method public hidebysig static int32 test_3_ldstarg_opt () cil managed
{
/* allocate first object */
newobj instance void class Tests/R1_class::.ctor ()
dup
ldc.i4.s 0x1337
/* stores '0x1337' in the field */
stfld int32 Tests/R1_class::test_field

dup
/* passing first object */
call void class Tests::ldstarg_opt_helper (Tests/R1_class)
/* should read '3' from first object */
ldfld int32 Tests/R1_class::test_field
ret
}


.method public hidebysig static int32 test_10_rconv_to_u8_ovf_un() cil managed
{
// Code size 20 (0x14)
Expand Down
2 changes: 2 additions & 0 deletions src/mono/mono/mini/interp/mintops.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ typedef enum {
#define MINT_IS_STLOC(op) ((op) >= MINT_STLOC_I1 && (op) <= MINT_STLOC_VT)
#define MINT_IS_MOVLOC(op) ((op) >= MINT_MOVLOC_1 && (op) <= MINT_MOVLOC_VT)
#define MINT_IS_STLOC_NP(op) ((op) >= MINT_STLOC_NP_I4 && (op) <= MINT_STLOC_NP_O)
#define MINT_IS_LDARG(op) ((op) >= MINT_LDARG_I1 && (op) <= MINT_LDARG_VT)
#define MINT_IS_STARG(op) ((op) >= MINT_STARG_I1 && (op) <= MINT_STARG_VT)
#define MINT_IS_CONDITIONAL_BRANCH(op) ((op) >= MINT_BRFALSE_I4 && (op) <= MINT_BLT_UN_R8_S)
#define MINT_IS_CALL(op) ((op) >= MINT_CALL && (op) <= MINT_JIT_CALL)
#define MINT_IS_PATCHABLE_CALL(op) ((op) >= MINT_CALL && (op) <= MINT_VCALL)
Expand Down
54 changes: 38 additions & 16 deletions src/mono/mono/mini/interp/transform.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,19 @@ typedef struct

#define STACK_VALUE_NONE 0
#define STACK_VALUE_LOCAL 1
#define STACK_VALUE_I4 2
#define STACK_VALUE_I8 3
#define STACK_VALUE_ARG 2
#define STACK_VALUE_I4 3
#define STACK_VALUE_I8 4

// StackValue contains data to construct an InterpInst that is equivalent with the contents
// of the stack slot / local.
// of the stack slot / local / argument.
typedef struct {
// Indicates the type of the stored information. It can be a local or a constant
// Indicates the type of the stored information. It can be a local, argument or a constant
int type;
// Holds the local index or the actual constant value
union {
int local;
int arg;
gint32 i;
gint64 l;
};
Expand All @@ -75,7 +77,7 @@ typedef struct {
typedef struct
{
// This indicates what is currently stored in this stack slot. This can be a constant
// or the copy of a local.
// or the copy of a local / argument.
StackValue val;
// The instruction that pushed this stack slot. If ins is null, we can't remove the usage
// of the stack slot, because we can't clear the instruction that set it.
Expand Down Expand Up @@ -6656,6 +6658,18 @@ clear_stack_content_info_for_local (StackContentInfo *start, StackContentInfo *e
}
}

// The value of argument has changed. This means the contents of the stack where the
// argument was loaded, no longer contain the value of the argument. Clear them.
static void
clear_stack_content_info_for_argument (StackContentInfo *start, StackContentInfo *end, int argument)
{
StackContentInfo *si;
for (si = start; si < end; si++) {
if (si->val.type == STACK_VALUE_ARG && si->val.arg == argument)
si->val.type = STACK_VALUE_NONE;
}
}

// The value of local has changed. This means we can no longer assume that any other local
// is a copy of this local.
static void
Expand Down Expand Up @@ -7114,7 +7128,7 @@ interp_cprop (TransformData *td)
} else {
locals [dest_local].type = STACK_VALUE_NONE;
}
} else if (sp->val.type == STACK_VALUE_NONE) {
} else if (sp->val.type == STACK_VALUE_NONE || sp->val.type == STACK_VALUE_ARG) {
locals [dest_local].type = STACK_VALUE_NONE;
} else {
g_assert (sp->val.type == STACK_VALUE_I4 || sp->val.type == STACK_VALUE_I8);
Expand Down Expand Up @@ -7183,6 +7197,15 @@ interp_cprop (TransformData *td)
// propagated instruction, so we remove the top of stack dependency
sp [-1].ins = NULL;
sp++;
} else if (MINT_IS_LDARG (ins->opcode)) {
sp->ins = ins;
sp->val.type = STACK_VALUE_ARG;
sp->val.arg = ins->opcode == MINT_LDARG_P0 ? 0 : ins->data [0];
sp++;
} else if (MINT_IS_STARG (ins->opcode)) {
int dest_arg = ins->data [0];
sp--;
clear_stack_content_info_for_argument (stack, sp, dest_arg);
} else if (ins->opcode >= MINT_BOX && ins->opcode <= MINT_BOX_NULLABLE) {
int offset = ins->data [1];
// Clear the stack slot that is boxed
Expand Down Expand Up @@ -7241,30 +7264,29 @@ interp_cprop (TransformData *td)
ins = interp_fold_binop (td, sp, ins);
sp--;
} else if (ins->opcode >= MINT_STFLD_I1 && ins->opcode <= MINT_STFLD_P && (mono_interp_opt & INTERP_OPT_SUPER_INSTRUCTIONS)) {
if (sp [-2].ins) {
InterpInst *obj_ins = sp [-2].ins;
if (obj_ins->opcode == MINT_LDLOC_O) {
int loc_index = obj_ins->data [0];
StackContentInfo *src = &sp [-2];
if (src->ins) {
if (src->val.type == STACK_VALUE_LOCAL) {
int loc_index = src->val.local;
int fld_offset = ins->data [0];
int mt = ins->opcode - MINT_STFLD_I1;
ins = interp_insert_ins (td, ins, MINT_STLOCFLD_I1 + mt);
ins->data [0] = loc_index;
ins->data [1] = fld_offset;
local_ref_count [loc_index]++;
interp_clear_ins (td, ins->prev);
interp_clear_ins (td, obj_ins);
interp_clear_ins (td, src->ins);
mono_interp_stats.super_instructions++;
mono_interp_stats.killed_instructions++;
} else if (obj_ins->opcode == MINT_LDARG_O || obj_ins->opcode == MINT_LDARG_P0) {
int arg_index = 0;
} else if (src->val.type == STACK_VALUE_ARG) {
int arg_index = src->val.arg;
int fld_offset = ins->data [0];
int mt = ins->opcode - MINT_STFLD_I1;
if (obj_ins->opcode == MINT_LDARG_O)
arg_index = obj_ins->data [0];
ins = interp_insert_ins (td, ins, MINT_STARGFLD_I1 + mt);
ins->data [0] = arg_index;
ins->data [1] = fld_offset;
interp_clear_ins (td, ins->prev);
interp_clear_ins (td, obj_ins);
interp_clear_ins (td, src->ins);
mono_interp_stats.super_instructions++;
mono_interp_stats.killed_instructions++;
}
Expand Down

0 comments on commit 53d4500

Please sign in to comment.