Re: [PATCH v2 09/30] tcg/loongarch64: Implement tcg_out_mov and tcg_out_movi

[WANG Xuerui <i.qemu@xen0n.name>]

Hi Richard,

On 9/22/21 12:25, Richard Henderson wrote:
> On 9/21/21 1:18 PM, WANG Xuerui wrote:
>> +    /* Test for PC-relative values that can be loaded faster.  */
>> +    intptr_t pc_offset = val - (uintptr_t)s->code_ptr;
>
> This isn't quite right for split r^x code buffer.
> You should have seen this with --enable-debug-tcg...
>
> You need pc_offset = tcg_pcrel_diff(s, (void *)val).
Indeed; I just realized TCG debugging isn't fully enabled with 
--enable-debug only. Will fix in v3.
>
>> +    if (pc_offset == (int32_t)pc_offset) {
>> +        tcg_target_long lo = sextreg(pc_offset, 0, 12);
>> +        tcg_target_long hi = pc_offset - lo;
>> +        tcg_out_opc_pcaddu12i(s, rd, hi >> 12);
>
> And... this doesn't quite work, right at the edges.  If lo is 
> negative, hi can overflow out of range.  There are a number of ways to 
> fix this.  One is to extract the pieces and re-assemble to see if it 
> matches.  Another is to rearrange the arithmetic just a little and use 
> PCALAU12I.
I actually wrote a small test program to test for this, but found no 
overflow issues here; rather the tcg_out_opc_ori call below has 
signedness problem (need to mask the low variable, which is signed, with 
0xfff to avoid overwriting the opcode field). I think I'll add a 
tcg_debug_assert here, but keep the logic intact.
>
>> +    tcg_target_long upper = (val >> 12) & 0xfffff;
>> +    tcg_target_long higher = (val >> 32) & 0xfffff;
>
> Better to use extract64(val, 12, 20) and extract64(val, 32, 30).
Sure; but as the instructions perform sign-extension, thus taking signed 
operands, sextract64 or the wrapped sextreg will do it.
>
>
> r~