Re: [Qemu-devel] [PATCH v3 26/35] target/riscv: Remove shift and slt insn manual decoding

[Bastian Koppelmann <kbastian@mail.uni-paderborn.de>]

On 11/1/18 4:59 PM, Palmer Dabbelt wrote:
> On Wed, 31 Oct 2018 15:38:08 PDT (-0700), richard.henderson@linaro.org 
> wrote:
>> On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>>>  static bool trans_slt(DisasContext *ctx, arg_slt *a)
>>>  {
>>> -    gen_arith(ctx, OPC_RISC_SLT, a->rd, a->rs1, a->rs2);
>>> +    TCGv source1 = tcg_temp_new();
>>> +    TCGv source2 = tcg_temp_new();
>>> +
>>> +    gen_get_gpr(source1, a->rs1);
>>> +    gen_get_gpr(source2, a->rs2);
>>> +
>>> +    tcg_gen_setcond_tl(TCG_COND_LT, source1, source1, source2);
>>
>> I do wonder about extracting this one line to gen_slt so that you can 
>> re-use
>> gen_arith and gen_arithi.
>>
>>> +    tcg_gen_setcond_tl(TCG_COND_LTU, source1, source1, source2);
>>
>> Similarly.
>>
>>>  static bool trans_sllw(DisasContext *ctx, arg_sllw *a)
>>>  {
>>> -    gen_arith(ctx, OPC_RISC_SLLW, a->rd, a->rs1, a->rs2);
>>> +    TCGv source1 = tcg_temp_new();
>>> +    TCGv source2 = tcg_temp_new();
>>> +
>>> +    gen_get_gpr(source1, a->rs1);
>>> +    gen_get_gpr(source2, a->rs2);
>>> +
>>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>>> +    tcg_gen_shl_tl(source1, source1, source2);
>>> +
>>> +    gen_set_gpr(a->rd, source1);
>>
>> Missing the ext32s after the shift.
>>
>>>  static bool trans_srlw(DisasContext *ctx, arg_srlw *a)
>>>  {
>>> -    gen_arith(ctx, OPC_RISC_SRLW, a->rd, a->rs1, a->rs2);
>>> +    TCGv source1 = tcg_temp_new();
>>> +    TCGv source2 = tcg_temp_new();
>>> +
>>> +    gen_get_gpr(source1, a->rs1);
>>> +    gen_get_gpr(source2, a->rs2);
>>> +
>>> +    /* clear upper 32 */
>>> +    tcg_gen_ext32u_tl(source1, source1);
>>> +    tcg_gen_andi_tl(source2, source2, 0x1F);
>>> +    tcg_gen_shr_tl(source1, source1, source2);
>>
>> Likewise.  (Consider source2 == 0.)
>>
>>> -    case OPC_RISC_SRL:
>>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>>> -        tcg_gen_shr_tl(source1, source1, source2);
>>> -        break;
>> ...
>>> -    case OPC_RISC_SRA:
>>> -        tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1);
>>> -        tcg_gen_sar_tl(source1, source1, source2);
>>> -        break;
>>
>> I see that the bugs are in the original though, so fixing them in a 
>> separate
>> patch is certainly ok.
>
> Since we're in the soft freeze now, I think it would be great to get 
> the bug fixes broken out as separate patches at the start of this 
> series that we can pick up for this release.  It would be great if the 
> decodetree conversion was a non-functional change, as that will make 
> it easier to review.


Sounds good. Unfortunately, I don't have much time right now. I try at 
least to extract the bugfixes, such that they make it for this release.


Cheers,

Bastian