Re: [PATCH v8 11/45] target/arm: Implement the ADDG, SUBG instructions

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 23 Jun 2020 at 20:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> +/*
> + * Add/subtract (immediate, with tags)
> + *
> + *  31 30 29 28         23 22 21     16 14      10 9   5 4   0
> + * +--+--+--+-------------+--+---------+--+-------+-----+-----+
> + * |sf|op| S| 1 0 0 0 1 0 |o2|  uimm6  |o3| uimm4 |  Rn | Rd  |
> + * +--+--+--+-------------+--+---------+--+-------+-----+-----+

Bit 23 should be '1'.

> + *
> + *    op: 0 -> add, 1 -> sub
> + */
> +static void disas_add_sub_imm_with_tags(DisasContext *s, uint32_t insn)
> +{
> +    int rd = extract32(insn, 0, 5);
> +    int rn = extract32(insn, 5, 5);
> +    int uimm4 = extract32(insn, 10, 4);
> +    int uimm6 = extract32(insn, 16, 6);
> +    bool sub_op = extract32(insn, 30, 1);
> +    TCGv_i64 tcg_rn, tcg_rd;
> +    int imm;
> +
> +    /* Test all of sf=1, S=0, o2=0, o3=0.  */
> +    if ((insn & 0xc040e000u) != 0x80000000u ||

This bit pattern doesn't seem to match the comment:
0xc is 0b1100 so that's sf and op, not sf and S;
0xe is 0b1110 so that's testing the top bit of uimm4
as well as op3. I think it should be 0xa040c000.
Though the existence of this bug suggests that it would
be clearer to test the individual fields :-)

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


Interestingly clang (6.0.0) gets pretty close to optimizing
the field test version:

bool foo(uint32_t insn) {
   return (insn & 0xa040c000) != 0x80000000u;
}

bool bar(uint32_t insn) {
   bool sf = extract32(insn, 31, 1);
   bool s = extract32(insn, 29, 1);
   bool o2 = extract32(insn, 22, 1);
   int op2 = extract32(insn, 14, 2);

   return sf != 1 || s != 0 || o2 != 0 || op2 != 0;
}

gives
0000000000000000 <foo>:
   0:   81 e7 00 c0 40 a0       and    $0xa040c000,%edi
   6:   81 ff 00 00 00 80       cmp    $0x80000000,%edi
   c:   0f 95 c0                setne  %al
   f:   c3                      retq

0000000000000010 <bar>:
  10:   89 f8                   mov    %edi,%eax
  12:   25 00 00 00 a0          and    $0xa0000000,%eax
  17:   3d 00 00 00 80          cmp    $0x80000000,%eax
  1c:   75 0a                   jne    28 <bar+0x18>
  1e:   f7 c7 00 c0 40 00       test   $0x40c000,%edi
  24:   0f 95 c0                setne  %al
  27:   c3                      retq
  28:   b0 01                   mov    $0x1,%al
  2a:   c3                      retq

(I don't know why it's split it into two tests: it's
not that it's testing "must be 1" in one part and "must
be 0" in the other because it has checked both sf and s
in the first comparison.)

gcc (7.4.0) makes more of a hash of it though:

  10:   89 f8                   mov    %edi,%eax
  12:   89 fa                   mov    %edi,%edx
  14:   c1 e8 1f                shr    $0x1f,%eax
  17:   c1 ea 1d                shr    $0x1d,%edx
  1a:   83 f0 01                xor    $0x1,%eax
  1d:   09 d0                   or     %edx,%eax
  1f:   83 e0 01                and    $0x1,%eax
  22:   75 13                   jne    37 <bar+0x27>
  24:   89 f8                   mov    %edi,%eax
  26:   c1 ef 16                shr    $0x16,%edi
  29:   c1 e8 0e                shr    $0xe,%eax
  2c:   83 e7 01                and    $0x1,%edi
  2f:   83 e0 03                and    $0x3,%eax
  32:   09 f8                   or     %edi,%eax
  34:   0f 95 c0                setne  %al
  37:   f3 c3                   repz retq

thanks
-- PMM