Re: [Qemu-devel] [PATCH 13/67] target/arm: Convert Data Processing (reg, reg-shifted-reg, imm)

[Richard Henderson]

On 7/29/19 8:25 AM, Peter Maydell wrote:
> I'm afraid this patch is too big for me to digest :-(
> 
> I just spent about half an hour trying to figure out whether
> the changes just to the thumb dp-immediate insns were right
> and didn't manage to work through it all.

Hmm.  It is probably the largest of the patches, and is caused by me trying to
do all of the insns handled by that one A32 code block.

I could perhaps split this to add fewer insns at a time, but I'd have to leave
the legacy decoder alone until the last patch, when it would go away all at
once.  I'm not sure if that is more or less reviewable.


> Why do we split it up into all these different kinds of patterns
> where some insns have special cases for rn==15 and some have
> special cases for rd==15 ?
> The legacy decoder doesn't seem to do that -- it treats everything
> the same.

It sorta special cases them, without actually diagnosing the UNPREDICTABLE
cases of invalid unused operands.

ARM MOV + MVN special cases:

> -        if (op1 != 0x0f && op1 != 0x0d) {
> -            rn = (insn >> 16) & 0xf;
> -            tmp = load_reg(s, rn);
> -        } else {
> -            tmp = NULL;
> -        }
...
> -        if (op1 != 0x0f && op1 != 0x0d) {
> -            tcg_temp_free_i32(tmp2);
> -        }

Here we have TST, TEQ, CMP, CMN:

> -        case 0x08:
> -            if (set_cc) {
> -                tcg_gen_and_i32(tmp, tmp, tmp2);
> -                gen_logic_CC(tmp);
> -            }
> -            tcg_temp_free_i32(tmp);
> -            break;
> -        case 0x09:
> -            if (set_cc) {
> -                tcg_gen_xor_i32(tmp, tmp, tmp2);
> -                gen_logic_CC(tmp);
> -            }
> -            tcg_temp_free_i32(tmp);
> -            break;
> -        case 0x0a:
> -            if (set_cc) {
> -                gen_sub_CC(tmp, tmp, tmp2);
> -            }
> -            tcg_temp_free_i32(tmp);
> -            break;
> -        case 0x0b:
> -            if (set_cc) {
> -                gen_add_CC(tmp, tmp, tmp2);
> -            }
> -            tcg_temp_free_i32(tmp);
> -            break;

I don't see bit 20 (set_cc) as even being optional in the decode, though rd is
RES0/SBZ and is thus ignoring it is valid for CONSTRAINED UNPREDICTABLE.

Thumb2 MOV, MVN (and the rest UNPREDICTABLE):

>              /* Data processing register constant shift.  */
> -            if (rn == 15) {
> -                tmp = tcg_temp_new_i32();
> -                tcg_gen_movi_i32(tmp, 0);
> -            } else {
> -                tmp = load_reg(s, rn);
> -            }
...
> -                if (rn == 15) {
> -                    tmp = tcg_temp_new_i32();
> -                    tcg_gen_movi_i32(tmp, 0);
> -                } else {
> -                    tmp = load_reg(s, rn);
> -                }

TST, TEQ, CMP, CMN (and the rest UNPREDICTABLE):

> -                } else if (rd != 15) {
> -                    store_reg(s, rd, tmp);
> -                } else {
> -                    tcg_temp_free_i32(tmp);
> -                }

r~