diff options
author | Kyungwoo Lee <kyulee@microsoft.com> | 2016-03-11 23:51:46 -0800 |
---|---|---|
committer | Kyungwoo Lee <kyulee@microsoft.com> | 2016-03-15 16:36:55 -0700 |
commit | 2300f42bdd35f7b971d6fbeb54d2678ea429ef34 (patch) | |
tree | 1a22980e591deafd9557b650fb3131a79b5a55c9 | |
parent | fd05aabce3f1191b485dbc00dd388219a1d400d2 (diff) | |
download | coreclr-2300f42bdd35f7b971d6fbeb54d2678ea429ef34.tar.gz coreclr-2300f42bdd35f7b971d6fbeb54d2678ea429ef34.tar.bz2 coreclr-2300f42bdd35f7b971d6fbeb54d2678ea429ef34.zip |
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).
-rw-r--r-- | src/jit/emit.h | 3 | ||||
-rw-r--r-- | src/jit/emitarm64.cpp | 57 | ||||
-rw-r--r-- | src/jit/emitfmtsarm64.h | 3 | ||||
-rw-r--r-- | src/jit/emitinl.h | 8 | ||||
-rw-r--r-- | src/jit/instrsarm64.h | 16 |
5 files changed, 48 insertions, 39 deletions
diff --git a/src/jit/emit.h b/src/jit/emit.h index c35532233e..39241278a5 100644 --- 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 8dfdf961bc..e4ad5c77b8 100644 --- 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 a2f86ddcc5..722e48c580 100644 --- 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 bb561fedee..7bbbf06998 100644 --- 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 8ecda575d0..21fddc5fe7 100644 --- 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 |