[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 18/27] target/mips: Extract MXU code to new mxu_translate.c fi
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PULL 18/27] target/mips: Extract MXU code to new mxu_translate.c file |
Date: |
Mon, 15 Mar 2021 23:43:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 |
On 3/15/21 10:33 PM, Peter Maydell wrote:
> On Sat, 13 Mar 2021 at 19:58, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Extract 1600+ lines from the big translate.c into a new file.
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> This code motion caused Coverity to rescan this code, and
> it thinks there's a problem in this function (CID 1450831).
> It looks to me like it might be right...
Oops, our mails crossed :)
I wonder if this is a simple rescan or if target/mips/translate.c
is too big and Coverity bails out on it by timeout (this is what
Coccinelle does). Now the extracted code could finally get processed.
>> +/*
>> + * D16MAX
>> + * Update XRa with the 16-bit-wise maximums of signed integers
>> + * contained in XRb and XRc.
>> + *
>> + * D16MIN
>> + * Update XRa with the 16-bit-wise minimums of signed integers
>> + * contained in XRb and XRc.
>> + */
>> +static void gen_mxu_D16MAX_D16MIN(DisasContext *ctx)
>> +{
>> + uint32_t pad, opc, XRc, XRb, XRa;
>> +
>> + pad = extract32(ctx->opcode, 21, 5);
>> + opc = extract32(ctx->opcode, 18, 3);
>> + XRc = extract32(ctx->opcode, 14, 4);
>> + XRb = extract32(ctx->opcode, 10, 4);
>> + XRa = extract32(ctx->opcode, 6, 4);
>> +
>> + if (unlikely(pad != 0)) {
>> + /* opcode padding incorrect -> do nothing */
>> + } else if (unlikely(XRc == 0)) {
>> + /* destination is zero register -> do nothing */
>> + } else if (unlikely((XRb == 0) && (XRa == 0))) {
>> + /* both operands zero registers -> just set destination to zero */
>> + tcg_gen_movi_i32(mxu_gpr[XRc - 1], 0);
>> + } else if (unlikely((XRb == 0) || (XRa == 0))) {
>
> In this block of code either XRb or XRa is zero...
>
>> + /* exactly one operand is zero register - find which one is not...*/
>> + uint32_t XRx = XRb ? XRb : XRc;
>> + /* ...and do half-word-wise max/min with one operand 0 */
>> + TCGv_i32 t0 = tcg_temp_new();
>> + TCGv_i32 t1 = tcg_const_i32(0);
>> +
>> + /* the left half-word first */
>> + tcg_gen_andi_i32(t0, mxu_gpr[XRx - 1], 0xFFFF0000);
>> + if (opc == OPC_MXU_D16MAX) {
>> + tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);
>> + } else {
>> + tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
>> + }
>
> but in these smax/smin calls we're clearly assuming that
> XRa is not zero.
>
> There seems to be some confusion over which registers are
> the inputs and which is the output. The top-level function
> comment says XRa is the input and XRb/XRc the inputs.
> But the "destination is zero register" comment is against
> a check on XRc, and the "both operands zero" comment is
> against a check on XRa and XRb, as is the "one operand
> is zero" comment...
>
>> +/*
>> + * Q8MAX
>> + * Update XRa with the 8-bit-wise maximums of signed integers
>> + * contained in XRb and XRc.
>> + *
>> + * Q8MIN
>> + * Update XRa with the 8-bit-wise minimums of signed integers
>> + * contained in XRb and XRc.
>> + */
>> +static void gen_mxu_Q8MAX_Q8MIN(DisasContext *ctx)
>> +{
>> + uint32_t pad, opc, XRc, XRb, XRa;
>> +
>> + pad = extract32(ctx->opcode, 21, 5);
>> + opc = extract32(ctx->opcode, 18, 3);
>> + XRc = extract32(ctx->opcode, 14, 4);
>> + XRb = extract32(ctx->opcode, 10, 4);
>> + XRa = extract32(ctx->opcode, 6, 4);
>> +
>> + if (unlikely(pad != 0)) {
>> + /* opcode padding incorrect -> do nothing */
>> + } else if (unlikely(XRa == 0)) {
>> + /* destination is zero register -> do nothing */
>> + } else if (unlikely((XRb == 0) && (XRc == 0))) {
>> + /* both operands zero registers -> just set destination to zero */
>> + tcg_gen_movi_i32(mxu_gpr[XRa - 1], 0);
>> + } else if (unlikely((XRb == 0) || (XRc == 0))) {
>> + /* exactly one operand is zero register - make it be the first...*/
>> + uint32_t XRx = XRb ? XRb : XRc;
>
> Contrast this function, where the code and the comments are
> all in agreement that XRa is destination and XRb/XRc inputs.
Yes, from the spec XRa is the destination register, XRb/XRc are
the compared inputs. Unfortunately I couldn't sort the function
body code so I ended rewriting it.
Regards,
Phil.
- [PULL 10/27] target/mips: Remove unused CPUMIPSState* from MXU functions, (continued)
- [PULL 10/27] target/mips: Remove unused CPUMIPSState* from MXU functions, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 11/27] target/mips: Pass instruction opcode to decode_opc_mxu(), Philippe Mathieu-Daudé, 2021/03/13
- [PULL 12/27] target/mips: Use OPC_MUL instead of OPC__MXU_MUL, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 13/27] target/mips: Move MUL opcode check from decode_mxu() to decode_legacy(), Philippe Mathieu-Daudé, 2021/03/13
- [PULL 14/27] target/mips: Rename decode_opc_mxu() as decode_ase_mxu(), Philippe Mathieu-Daudé, 2021/03/13
- [PULL 15/27] target/mips: Convert decode_ase_mxu() to decodetree prototype, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 16/27] target/mips: Simplify decode_opc_mxu() ifdef'ry, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 17/27] target/mips: Introduce mxu_translate_init() helper, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 18/27] target/mips: Extract MXU code to new mxu_translate.c file, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 19/27] target/mips: Use gen_load_gpr[_hi]() when possible, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 21/27] target/mips/tx79: Move MTHI1 / MTLO1 opcodes to decodetree, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 20/27] target/mips/tx79: Move MFHI1 / MFLO1 opcodes to decodetree, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 22/27] target/mips/translate: Make gen_rdhwr() public, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 23/27] target/mips/translate: Simplify PCPYH using deposit_i64(), Philippe Mathieu-Daudé, 2021/03/13
- [PULL 24/27] target/mips/tx79: Move PCPYH opcode to decodetree, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 25/27] target/mips/tx79: Move PCPYLD / PCPYUD opcodes to decodetree, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 26/27] target/mips: Remove 'C790 Multimedia Instructions' dead code, Philippe Mathieu-Daudé, 2021/03/13
- [PULL 27/27] target/mips/tx79: Salvage instructions description comment, Philippe Mathieu-Daudé, 2021/03/13
- Re: [PULL 00/27] MIPS patches for 2021-03-13, Peter Maydell, 2021/03/15