[interp] add guard that a local hasn't been modified for stlocfld fusion (mono/mono...
authorBernhard Urban-Forster <lewurm@gmail.com>
Thu, 19 Dec 2019 01:05:57 +0000 (02:05 +0100)
committermonojenkins <jo.shields+jenkins@xamarin.com>
Thu, 19 Dec 2019 01:05:57 +0000 (02:05 +0100)
[interp] add guard that a local hasn't been modified for stlocfld fusion

Fixes https://github.com/mono/mono/issues/18120

Commit migrated from https://github.com/mono/mono/commit/e7b98d3587fddd51db69baddb370321cca521d60

src/mono/mono/mini/iltests.il
src/mono/mono/mini/interp/mintops.h
src/mono/mono/mini/interp/transform.c

index e361af3..4ee7e2c 100644 (file)
@@ -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)
index 9e3c3b6..4164733 100644 (file)
@@ -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)
index 9379f37..0da8b1c 100644 (file)
@@ -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;
        };
@@ -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.
@@ -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
@@ -7114,7 +7128,7 @@ retry:
                                } 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);
@@ -7183,6 +7197,15 @@ retry:
                        // 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
@@ -7241,30 +7264,29 @@ retry:
                        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++;
                                }