|
From: | WANG Xuerui |
Subject: | Re: [PATCH v5 09/30] tcg/loongarch64: Implement tcg_out_mov and tcg_out_movi |
Date: | Sun, 26 Sep 2021 00:47:27 +0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:94.0) Gecko/20100101 Thunderbird/94.0a1 |
Hi Philippe, On 9/25/21 17:54, Philippe Mathieu-Daudé wrote:
On 9/24/21 19:25, WANG Xuerui wrote:Signed-off-by: WANG Xuerui <git@xen0n.name> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- tcg/loongarch64/tcg-target.c.inc | 109 +++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+)+/* Loads a 32-bit immediate into rd, sign-extended. */ +static void tcg_out_movi_i32(TCGContext *s, TCGReg rd, int32_t val) +{ + /* Single-instruction cases. */ + tcg_target_long lo = sextreg(val, 0, 12); + if (lo == val) { + /* val fits in simm12: addi.w rd, zero, val */ + tcg_out_opc_addi_w(s, rd, TCG_REG_ZERO, val); + return; + } + if (0x800 <= val && val <= 0xfff) { + /* val fits in uimm12: ori rd, zero, val */ + tcg_out_opc_ori(s, rd, TCG_REG_ZERO, val); + return; + } + + /* High bits must be set; load with lu12i.w + optional ori. */ + tcg_target_long hi12 = sextreg(val, 12, 20);Please declare variables in function prologue.
Sure; will fix in v6.
I added ASCII art to hopefully clarify the namings; originally I used MIPS R6 terms (low, upper, higher, top) but all the MIPS R6 instructions take 16-bit imm, so I figured just naming by bitfield LSB index would be best. The Loongson documentation people didn't invent any dedicated name for the 4 parts or 3 load-upper instructions, either.Maybe name lo12 and hi20?
As Richard explained, lo is signed while ori takes unsigned imm, so this is necessary to not trip up the debug assert and overwrite the opcode.+ tcg_out_opc_lu12i_w(s, rd, hi12); + if (lo != 0) { + tcg_out_opc_ori(s, rd, rd, lo & 0xfff);Isn't lo already 12-bit? Why the mask?
+ } +} + +static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd, + tcg_target_long val) +{ + if (type == TCG_TYPE_I32 || val == (int32_t)val) { + tcg_out_movi_i32(s, rd, val); + return; + } + + /* PC-relative cases. */ + intptr_t pc_offset = tcg_pcrel_diff(s, (void *)val);Declare in prologue.
Ack; will fix.
Nice catch; that was leftover from v4 where the tcg_out_movi_i32 logic was not factored out. Fixed in v6.+ if (pc_offset == sextreg(pc_offset, 0, 22) && (pc_offset & 3) == 0) {+ /* Single pcaddu2i. */ + tcg_out_opc_pcaddu2i(s, rd, pc_offset >> 2); + return; + } + + if (pc_offset == (int32_t)pc_offset) { + /* Offset within 32 bits; load with pcalau12i + ori. */ + tcg_target_long lo = sextreg(val, 0, 12);Name this 'val_lo' similarly to val_hi?
+ tcg_target_long pc_hi = (val - pc_offset) >> 12; + tcg_target_long val_hi = val >> 12; + tcg_target_long offset_hi = val_hi - pc_hi; + tcg_debug_assert(offset_hi == sextreg(offset_hi, 0, 20)); + tcg_out_opc_pcalau12i(s, rd, offset_hi); + if (lo != 0) { + tcg_out_opc_ori(s, rd, rd, lo & 0xfff);Again, lo is already 12-bit.
Same as above.
Hmm, comparison operators return boolean results already; and I thought expressions like `foo ? true : false` are typically considered to have "bad smell"? I think I'd want to keep the current way of saying things... But I'll of course move the declaration to function prologue.+ } + return; + } + + /* Single cu52i.d case. */ + if (ctz64(val) >= 52) { + tcg_out_opc_cu52i_d(s, rd, TCG_REG_ZERO, val >> 52); + return; + } ++ /* Slow path. Initialize the low 32 bits, then concat high bits. */+ tcg_out_movi_i32(s, rd, val); + + bool rd_high_bits_are_ones = (int32_t)val < 0;Declare in prologue, however this is hard to read. KISS: rd_high_bits_are_ones = (int32_t)val < 0 ? true : false;
The names are getting long with addition of bitfield lengths; I hope the ASCII art in v6 could alleviate the need for longer names.+ tcg_target_long hi32 = sextreg(val, 32, 20);By 'hi32' I expect the 32 high bits. Maybe explicit as hi32_20?+ tcg_target_long hi52 = sextreg(val, 52, 12);And hi52_12?
As explained by Richard, this is indeed meant to be an unconditional overwrite. After concatenating the 51-to-32 bits, the topmost 12 bits is changed to be sign-extension of hi32, so the flag must be updated to reflect that.+ + if (imm_part_needs_loading(rd_high_bits_are_ones, hi32)) { + tcg_out_opc_cu32i_d(s, rd, hi32); + rd_high_bits_are_ones = hi32 < 0;Again KISS: if (hi32 < 0) { rd_high_bits_are_ones = true; }
+ } + + if (imm_part_needs_loading(rd_high_bits_are_ones, hi52)) { + tcg_out_opc_cu52i_d(s, rd, rd, hi52); + } +}With comment addressed: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
[Prev in Thread] | Current Thread | [Next in Thread] |