Re: [Qemu-devel] [PATCH v2 12/22] target-mips: add ALIGN, DALIGN, BITSWAP and DBITSWAP instructions

[Leon Alrae]

On 11/06/2014 17:39, Richard Henderson wrote:
> On 06/11/2014 08:19 AM, Leon Alrae wrote:
>> * add missing zero register case
> 
> What missing zero register case?
> 
>> +        if (rd == 0) {
>> +            /* Treat as NOP. */
>> +            break;
>> +        }
> 
> This is normally handled by gen_store_gpr...
> 
>> +            if (rt == 0) {
>> +                tcg_gen_movi_tl(cpu_gpr[rd], 0);
>> +            } else {
>> +                gen_helper_dbitswap(cpu_gpr[rd], cpu_gpr[rt]);
>> +            }
>> +            break;
> 
> ... and this is normally handed by gen_load_gpr.
> 
> Open-coding these tests just clutters the code, making it harder to read.  
> C.f.
> the 1500 lines removed during a cleanup of target-alpha for exactly this sort
> of thing.

Yes, it clutters the code a bit. However, by looking at the other
instructions I got the impression that preferable approach is to avoid
generating unnecessary code if we can. This seems to be sensible so I'm
trying to stick to it. So in bitswap example I think there is no point
in calling the helper if we know that the result will be 0.

Regards,
Leon