ARM64: Revisit for Fix GC hole in indirect call site

This fixes https://github.com/dotnet/coreclr/issues/3738.
The fix I made in
https://github.com/dotnet/coreclr/commit/4dfd323dab88b902fc9479efa60cb5d6b7659e94
 used 3rd/4th operand to keep GC info, which actually conflicts with
 address field which is unioned with these operands.
So, I go back to the original fix that I proposed below:

Indirect call (```br``` or ```blr```) target is encoded with a register
which the first operand internally represents.
Unfortunately, call sites use the first two operands to hold GC callee-save registers.
So, this GC register information was overridden by the call target operand
in the indirect(virtual) call sites.
The fix is to split branch instruction categories for these two instructions
while keeping ```ret``` same as before. They internally use the third
operand to encode the target since the first two are used for GC info.
The reason I didn't change ```ret``` is because the return instruction
is created as a small instruction (not using operand 3 and more).

diff --git a/src/jit/emit.h b/src/jit/emit.h
index c355322..3924127 100644 (file)
--- a/src/jit/emit.h
+++ b/src/jit/emit.h
@@ -942,9 +942,6 @@ protected:
                 iiaEncodedInstrCount = (count << iaut_SHIFT) | iaut_INST_COUNT;
             }
 
-            // Note that we use the _idReg3 and _idReg4 fields to hold
-            // the live gcrefReg mask for the call instructions on arm64
-            //
             struct
             {
                 regNumber    _idReg3       :REGNUM_BITS;

diff --git a/src/jit/emitarm64.cpp b/src/jit/emitarm64.cpp
index 8dfdf96..e4ad5c7 100644 (file)
--- a/src/jit/emitarm64.cpp
+++ b/src/jit/emitarm64.cpp
@@ -175,6 +175,10 @@ void                emitter::emitInsSanityCheck(instrDesc *id)
         assert(isGeneralRegister(id->idReg1()));
         break;
 
+    case IF_BR_1B:    // BR_1B   ................ ......nnnnn.....         Rn
+        assert(isGeneralRegister(id->idReg3()));
+        break;
+
     case IF_LS_1A:    // LS_1A   .X......iiiiiiii iiiiiiiiiiittttt      Rt    PC imm(1MB)
         assert(isGeneralRegister(id->idReg1()) ||
                isVectorRegister(id->idReg1()));
@@ -1792,6 +1796,7 @@ emitter::code_t emitter::emitInsCode(instruction ins, insFormat fmt)
     case IF_BI_1A:
     case IF_BI_1B:
     case IF_BR_1A:
+    case IF_BR_1B:
     case IF_LS_1A:
     case IF_LS_2A:
     case IF_LS_2B:
@@ -3266,31 +3271,29 @@ void                emitter::emitIns_R(instruction ins,
     case INS_br:
         assert(isGeneralRegister(reg));
         id = emitNewInstrCns(attr, 0);
-        fmt = IF_BR_1A;
+        id->idReg3(reg);
+        fmt = IF_BR_1B;
         break;
 
     case INS_ret:
         assert(isGeneralRegister(reg));
         id = emitNewInstrSmall(attr);
+        id->idReg1(reg);
         fmt = IF_BR_1A;
         break;
 
     default:
-        // TODO-Cleanup: add unreached() here
-        break;
-
+        unreached();
     }
+
     assert(fmt != IF_NONE);
 
-    if (id != nullptr)
-    {
-        id->idIns(ins);
-        id->idInsFmt(fmt);
-        id->idReg1(reg);
+    id->idIns(ins);
+    id->idInsFmt(fmt);
+
+    dispIns(id);
+    appendToCurIG(id);
 
-        dispIns(id);
-        appendToCurIG(id);
-    }
 }
 
 /*****************************************************************************
@@ -6846,12 +6849,12 @@ void                emitter::emitIns_Call(EmitCallType  callType,
             {
                 ins = INS_blr;     // INS_blr Reg
             }
-            fmt = IF_BR_1A;
+            fmt = IF_BR_1B;
 
             id->idIns(ins);
             id->idInsFmt(fmt); 
 
-            id->idReg1(ireg);
+            id->idReg3(ireg);
             assert(xreg == REG_NA);
             break;
 
@@ -8215,18 +8218,21 @@ size_t              emitter::emitOutputInstr(insGroup  *ig,
 
     case IF_BR_1A:    // BR_1A   ................ ......nnnnn.....         Rn
         assert(insOptsNone(id->idInsOpt()));
+        assert(ins == INS_ret);
         code  = emitInsCode(ins, fmt);
         code |= insEncodeReg_Rn(id->idReg1());               // nnnnn
 
-        if (ins == INS_ret)
-        {
-            dst  += emitOutput_Instr(dst, code);
-        }
-        else  // INS_blr or INS_br
-        {
-             sz    = id->idIsLargeCall() ? sizeof(instrDescCGCA) : sizeof(instrDesc);
-             dst  += emitOutputCall(ig, dst, id, code);
-        }
+        dst  += emitOutput_Instr(dst, code);
+        break;
+
+    case IF_BR_1B:    // BR_1B   ................ ......nnnnn.....         Rn
+        assert(insOptsNone(id->idInsOpt()));
+        assert(ins != INS_ret);
+        code = emitInsCode(ins, fmt);
+        code |= insEncodeReg_Rn(id->idReg3());               // nnnnn
+
+        sz = id->idIsLargeCall() ? sizeof(instrDescCGCA) : sizeof(instrDesc);
+        dst += emitOutputCall(ig, dst, id, code);
         break;
 
     case IF_LS_1A:    // LS_1A   XX......iiiiiiii iiiiiiiiiiittttt      Rt    PC imm(1MB)
@@ -9909,6 +9915,11 @@ void                emitter::emitDispIns(instrDesc *  id,
         emitDispReg(id->idReg1(), size, false);
         break;
 
+    case IF_BR_1B:    // BR_1B   ................ ......nnnnn.....         Rn
+        assert(insOptsNone(id->idInsOpt()));
+        emitDispReg(id->idReg3(), size, false);
+        break;
+
     case IF_LS_1A:    // LS_1A   XX......iiiiiiii iiiiiiiiiiittttt      Rt    PC imm(1MB)
         assert(insOptsNone(id->idInsOpt()));
         emitDispReg(id->idReg1(), size, true);

diff --git a/src/jit/emitfmtsarm64.h b/src/jit/emitfmtsarm64.h
index a2f86dd..722e48c 100644 (file)
--- a/src/jit/emitfmtsarm64.h
+++ b/src/jit/emitfmtsarm64.h
@@ -118,7 +118,8 @@ IF_DEF(BI_0B,       IS_NONE,               JMP)      // BI_0B   ......iiiiiiiiii
 IF_DEF(BI_0C,       IS_NONE,               CALL)     // BI_0C   ......iiiiiiiiii iiiiiiiiiiiiiiii               simm26:00   bl
 IF_DEF(BI_1A,       IS_NONE,               JMP)      // BI_1A   X.......iiiiiiii iiiiiiiiiiittttt      Rt       simm19:00   cbz cbnz
 IF_DEF(BI_1B,       IS_NONE,               JMP)      // BI_1B   B.......bbbbbiii iiiiiiiiiiittttt      Rt imm6  simm14:00   tbz tbnz
-IF_DEF(BR_1A,       IS_NONE,               CALL)     // BR_1A   ................ ......nnnnn.....         Rn                br blr ret
+IF_DEF(BR_1A,       IS_NONE,               CALL)     // BR_1A   ................ ......nnnnn.....         Rn                ret
+IF_DEF(BR_1B,       IS_NONE,               CALL)     // BR_1B   ................ ......nnnnn.....         Rn                br blr
 
 IF_DEF(LS_1A,       IS_NONE,               JMP)      // LS_1A   .X......iiiiiiii iiiiiiiiiiittttt      Rt    PC imm(1MB)
 IF_DEF(LS_2A,       IS_NONE,               NONE)     // LS_2A   .X.......X...... ......nnnnnttttt      Rt Rn

diff --git a/src/jit/emitinl.h b/src/jit/emitinl.h
index bb561fe..7bbbf06 100644 (file)
--- a/src/jit/emitinl.h
+++ b/src/jit/emitinl.h
@@ -332,7 +332,7 @@ ssize_t             emitter::emitGetInsAmdAny(instrDesc *id)
      if ((regmask & RBM_R23) != RBM_NONE)
          encodeMask |= 0x10;
 
-     id->idReg3((regNumber)encodeMask);  // Save in idReg3
+     id->idReg1((regNumber)encodeMask);  // Save in idReg1
 
      encodeMask = 0;
 
@@ -347,7 +347,7 @@ ssize_t             emitter::emitGetInsAmdAny(instrDesc *id)
      if ((regmask & RBM_R28) != RBM_NONE)
          encodeMask |= 0x10;
 
-     id->idReg4((regNumber)encodeMask);  // Save in idReg4
+     id->idReg2((regNumber)encodeMask);  // Save in idReg2
 
 #else
     NYI("unknown target");
@@ -419,7 +419,7 @@ ssize_t             emitter::emitGetInsAmdAny(instrDesc *id)
 
 #elif defined(_TARGET_ARM64_)
     assert(REGNUM_BITS >= 5);
-    encodeMask = id->idReg3();
+    encodeMask = id->idReg1();
 
     if ((encodeMask & 0x01) != 0)
         regmask |= RBM_R19;
@@ -432,7 +432,7 @@ ssize_t             emitter::emitGetInsAmdAny(instrDesc *id)
     if ((encodeMask & 0x10) != 0)
         regmask |= RBM_R23;
 
-    encodeMask = id->idReg4();
+    encodeMask = id->idReg2();
 
     if ((encodeMask & 0x01) != 0)
         regmask |= RBM_R24;

diff --git a/src/jit/instrsarm64.h b/src/jit/instrsarm64.h
index 8ecda57..21fddc5 100644 (file)
--- a/src/jit/instrsarm64.h
+++ b/src/jit/instrsarm64.h
@@ -597,15 +597,15 @@ INST1(bl_local,"bl",     0, 0, IF_BI_0A,  0x94000000)
 INST1(bl,      "bl",     0, 0, IF_BI_0C,  0x94000000)
                                    //  bl      simm26               BI_0C  100101iiiiiiiiii iiiiiiiiiiiiiiii   9400 0000   simm26:00
 
-INST1(br,      "br",     0, 0, IF_BR_1A,  0xD61F0000)
-                                   //  br      Rn                   BR_1A  1101011000011111 000000nnnnn00000   D61F 0000   
-   
-INST1(blr,     "blr",    0, 0, IF_BR_1A,  0xD63F0000)
-                                   //  blr     Rn                   BR_1A  1101011000111111 000000nnnnn00000   D63F 0000   
-   
+INST1(br,      "br",     0, 0, IF_BR_1B,  0xD61F0000)
+                                   //  br      Rn                   BR_1B  1101011000011111 000000nnnnn00000   D61F 0000
+
+INST1(blr,     "blr",    0, 0, IF_BR_1B,  0xD63F0000)
+                                   //  blr     Rn                   BR_1B  1101011000111111 000000nnnnn00000   D63F 0000
+
 INST1(ret,     "ret",    0, 0, IF_BR_1A,  0xD65F0000)
-                                   //  ret     Rn                   BR_1A  1101011001011111 000000nnnnn00000   D65F 0000   
-   
+                                   //  ret     Rn                   BR_1A  1101011001011111 000000nnnnn00000   D65F 0000
+
 INST1(beq,     "beq",    0, 0, IF_BI_0B,  0x54000000)  
                                    //  beq     simm19               BI_0B  01010100iiiiiiii iiiiiiiiiii00000   5400 0000   simm19:00