[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: |
Peter Maydell |
Subject: |
Re: [PULL 18/27] target/mips: Extract MXU code to new mxu_translate.c file |
Date: |
Mon, 15 Mar 2021 21:33:45 +0000 |
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...
> +/*
> + * 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.
thanks
-- PMM
- [PULL 09/27] target/mips: Remove XBurst Media eXtension Unit dead code, (continued)
- [PULL 09/27] target/mips: Remove XBurst Media eXtension Unit dead code, Philippe Mathieu-Daudé, 2021/03/13
- [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
- Re: [PULL 18/27] target/mips: Extract MXU code to new mxu_translate.c file,
Peter Maydell <=
- [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