Re: [PATCH 2/2] tcg/loongarch64: Add direct jump support

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

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]


On 2022/10/12 19:34, WANG Xuerui wrote:
>> +    tcg_insn_unit i1, i2;
>> +
>> +    ptrdiff_t offset = addr - jmp_rx;
>> +
>> +    if (offset == sextreg(offset, 0, 28)) {
>> +        i1 = OPC_B | ((offset >> 18) & 0x3ff) | ((offset << 8) & 
>> 0x3fffc00);
> Use encode_sd10k16_insn instead. No need to juggle the bits yourself 
> and you get nice range assertion for free.
>> +        i2 = NOP; 

One more thing. I think there's a certain trend of making sure 
potentially bad things don't happen and stopping speculative insn 
fetch/execution by making the NOP here a BREAK instead. Although I'm not 
sure if LoongArch or TCG is affected by the various speculation 
vulnerabilities out there, to any degree, it might not hurt to just make 
it a BREAK? Performance shouldn't get affected in any way but one could 
probably sleep better with a guaranteed termination of execution flow 
after the B.

And you should change the "tcg_out32(s, NOP)" some lines above into just 
a "tcg_out_andi(s, 0, 0, 0)". This way you wouldn't have to define a NOP 
constant at all... Or, maybe better, make a "tcg_out_nop" helper right 
after the insn-defs include and after the "TCG intrinsics" section (it 
isn't one, but there isn't a better place), then use it here, so you get 
to include some more comments while still not having to define a NOP. Like:

static void tcg_out_nop(TCGContext *s)
{
	/* Conventional NOP in LoongArch is `andi zero, zero, 0`.  */
	tcg_out_opc_andi(s, 0, 0, 0);
}

[-- Attachment #2: Type: text/html, Size: 2170 bytes --]