| Message ID | 1fe9630dd4ad73b8581684cd0ed6d5fa2918390f.1542321076.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add RISC-V TCG backend support | expand |
On 11/15/18 11:35 PM, Alistair Francis wrote: > +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > +{ > + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; > + tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1); > + > + code_ptr[0] |= encode_sbimm12(offset); > +} FYI, I have an in-flight patch for 4.0 that will make patch_reloc return a bool for relocation success, which will move these asserts. http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02237.html > +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > +{ > + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; > + tcg_debug_assert(offset == (int32_t)offset); > + > + int32_t hi20 = ((offset + 0x800) >> 12) << 12; > + int32_t lo12 = offset - hi20; > + > + code_ptr[0] |= encode_uimm20(hi20); > + code_ptr[1] |= encode_imm12(lo12); > +} > + This is ok for patching during generation, but it is not ok for tb_target_set_jmp_target from patch 9. Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit? If not, you may be just better off using the indirect method. r~
On Fri, Nov 16, 2018 at 12:33 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 11/15/18 11:35 PM, Alistair Francis wrote: > > +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > > +{ > > + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; > > + tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1); > > + > > + code_ptr[0] |= encode_sbimm12(offset); > > +} > > FYI, I have an in-flight patch for 4.0 that will make patch_reloc return a bool > for relocation success, which will move these asserts. > > http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02237.html Thanks, I'll keep an eye on this. > > > > +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target) > > +{ > > + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; > > + tcg_debug_assert(offset == (int32_t)offset); > > + > > + int32_t hi20 = ((offset + 0x800) >> 12) << 12; > > + int32_t lo12 = offset - hi20; > > + > > + code_ptr[0] |= encode_uimm20(hi20); > > + code_ptr[1] |= encode_imm12(lo12); > > +} > > + > > This is ok for patching during generation, but it is not ok for > tb_target_set_jmp_target from patch 9. > > Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit? > If not, you may be just better off using the indirect method. I'm not sure. Is the indirect method just using atomic set, because that is what I have now? Alistair > > > r~
On 11/21/18 2:15 AM, Alistair Francis wrote: >> Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit? >> If not, you may be just better off using the indirect method. > > I'm not sure. Is the indirect method just using atomic set, because > that is what I have now? It's controlled by TCG_TARGET_HAS_direct_jump, and goes through here: > + case INDEX_op_goto_tb: > + if (s->tb_jmp_insn_offset) { > + /* direct jump method */ > + s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s); > + /* should align on 64-bit boundary for atomic patching */ > + tcg_out_opc_upper(s, OPC_AUIPC, TCG_REG_TMP0, 0); > + tcg_out_opc_imm(s, OPC_JALR, TCG_REG_ZERO, TCG_REG_TMP0, 0); > + } else { > + /* indirect jump method */ > + tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP0, TCG_REG_ZERO, > + (uintptr_t)(s->tb_jmp_target_addr + a0)); > + tcg_out_opc_imm(s, OPC_JALR, TCG_REG_ZERO, TCG_REG_TMP0, 0); > + } > + s->tb_jmp_reset_offset[a0] = tcg_current_code_size(s); > + break; where as you've correctly implemented, the indirect jump loads a destination from memory (atomically, via a single load insn), and then branches to it. The direct jump method avoids the load from memory, but then one has to be able to modify the insn(s) that perform the jump with a single atomic_set. Which in this case, means that the two insns must be aligned so that we can perform a single aligned 64-bit store. I recommend at least starting with the indirect method because it's easier. r~
On Tue, 20 Nov 2018 17:15:11 PST (-0800), alistair23@gmail.com wrote: > On Fri, Nov 16, 2018 at 12:33 AM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 11/15/18 11:35 PM, Alistair Francis wrote: >> > +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target) >> > +{ >> > + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; >> > + tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1); >> > + >> > + code_ptr[0] |= encode_sbimm12(offset); >> > +} >> >> FYI, I have an in-flight patch for 4.0 that will make patch_reloc return a bool >> for relocation success, which will move these asserts. >> >> http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02237.html > > Thanks, I'll keep an eye on this. > >> >> >> > +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target) >> > +{ >> > + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; >> > + tcg_debug_assert(offset == (int32_t)offset); >> > + >> > + int32_t hi20 = ((offset + 0x800) >> 12) << 12; >> > + int32_t lo12 = offset - hi20; >> > + >> > + code_ptr[0] |= encode_uimm20(hi20); >> > + code_ptr[1] |= encode_imm12(lo12); >> > +} >> > + >> >> This is ok for patching during generation, but it is not ok for >> tb_target_set_jmp_target from patch 9. >> >> Will the riscv-32 compiler use a FSTD insn to implement atomic_set for 64-bit? >> If not, you may be just better off using the indirect method. > > I'm not sure. Is the indirect method just using atomic set, because > that is what I have now? Per the memory model chapter (which is being ratified now), FSD is not atomic on rv32i: An FLD or FSD instruction for which XLEN<64 may also be decomposed into a set of component memory operations of any granularity. So they should not be used on RV32I hosts to provide atomicity, even if they may be atomic on some microarchitectures.
On 11/21/18 4:53 PM, Palmer Dabbelt wrote: > > Per the memory model chapter (which is being ratified now), FSD is not atomic > on rv32i: > > An FLD or FSD instruction for which XLEN<64 may also be decomposed into a > set of component memory operations of any granularity. Ah, thanks. I knew I should have checked the manual. Anyway, that means *only* the indirect goto_tb method is viable for rv32. r~
diff --git a/tcg/riscv/tcg-target.inc.c b/tcg/riscv/tcg-target.inc.c index d402e48cbf..475feca906 100644 --- a/tcg/riscv/tcg-target.inc.c +++ b/tcg/riscv/tcg-target.inc.c @@ -371,6 +371,57 @@ static void tcg_out_opc_jump(TCGContext *s, RISCVInsn opc, tcg_out32(s, encode_uj(opc, rd, imm)); } +/* + * Relocations + */ + +static void reloc_sbimm12(tcg_insn_unit *code_ptr, tcg_insn_unit *target) +{ + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; + tcg_debug_assert(offset == sextract32(offset, 1, 12) << 1); + + code_ptr[0] |= encode_sbimm12(offset); +} + +static void reloc_jimm20(tcg_insn_unit *code_ptr, tcg_insn_unit *target) +{ + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; + tcg_debug_assert(offset == sextract32(offset, 1, 20) << 1); + + code_ptr[0] |= encode_ujimm12(offset); +} + +static void reloc_call(tcg_insn_unit *code_ptr, tcg_insn_unit *target) +{ + intptr_t offset = (intptr_t)target - (intptr_t)code_ptr; + tcg_debug_assert(offset == (int32_t)offset); + + int32_t hi20 = ((offset + 0x800) >> 12) << 12; + int32_t lo12 = offset - hi20; + + code_ptr[0] |= encode_uimm20(hi20); + code_ptr[1] |= encode_imm12(lo12); +} + +static void patch_reloc(tcg_insn_unit *code_ptr, int type, + intptr_t value, intptr_t addend) +{ + tcg_debug_assert(addend == 0); + switch (type) { + case R_RISCV_BRANCH: + reloc_sbimm12(code_ptr, (tcg_insn_unit *)value); + break; + case R_RISCV_JAL: + reloc_jimm20(code_ptr, (tcg_insn_unit *)value); + break; + case R_RISCV_CALL: + reloc_call(code_ptr, (tcg_insn_unit *)value); + break; + default: + tcg_abort(); + } +} + void tb_target_set_jmp_target(uintptr_t tc_ptr, uintptr_t jmp_addr, uintptr_t addr) {