Add CMP2 instruction

This patch aims to add CMP2 instruction for m68k family.

Description: Compares the value in Rn to each bound. The effective address contains the
bounds pair: upper bound following the lower bound. For signed comparisons, the
arithmetically smaller value should be used as the lower bound. For unsigned
comparisons, the logically smaller value should be the lower bound.
The size of the data and the bounds can be specified as byte, word, or long. If Rn is a
data register and the operation size is byte or word, only the appropriate low-order part
of Rn is checked. If Rn is an address register and the operation size is byte or word,
the bounds operands are sign-extended to 32 bits, and the resultant operands are
compared to the full 32 bits of An.
If the upper bound equals the lower bound, the valid range is a single value.


signed-off-by: Guolei <guol-fnst@cn.fujitsu.com <mailto:guol-fnst@cn.fujitsu.com%20> >


---
target-m68k/translate.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)

     INSN(bitop_reg, 01c0, f1c0, CF_ISA_A);
     INSN(arith_im,  0280, fff8, CF_ISA_A);
     INSN(byterev,   02c0, fff8, CF_ISA_APLUSC);
+    INSN(cmp2,      02c0, ffff, CF_ISA_A);
     INSN(arith_im,  0480, fff8, CF_ISA_A);
     INSN(ff1,       04c0, fff8, CF_ISA_APLUSC);
     INSN(arith_im,  0680, fff8, CF_ISA_A);
--
1.7.9.5

Guo, Lei <guol-fnst@cn.fujitsu.com> writes:

> This patch aims to add CMP2 instruction for m68k family.
>
> Description: Compares the value in Rn to each bound. The effective address contains the
> bounds pair: upper bound following the lower bound. For signed comparisons, the
> arithmetically smaller value should be used as the lower bound. For unsigned
> comparisons, the logically smaller value should be the lower bound.
> The size of the data and the bounds can be specified as byte, word, or long. If Rn is a
> data register and the operation size is byte or word, only the appropriate low-order part
> of Rn is checked. If Rn is an address register and the operation size is byte or word,
> the bounds operands are sign-extended to 32 bits, and the resultant operands are
> compared to the full 32 bits of An.
> If the upper bound equals the lower bound, the valid range is a single value.
>
>
> signed-off-by: Guolei <guol-fnst@cn.fujitsu.com
> <mailto:guol-fnst@cn.fujitsu.com%20> >

incorrect Signed-off-by: format

>
> ---
> target-m68k/translate.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)

I'm fairly sure this fails scripts/checkpatch.pl but I was unable to run
it as your email seems to be html with a base64 uuencoded blob in it.
Please check your patch with checkpatch.pl before re-sending.

Also please fix your mailer to send plain text emails or better yet use
git-send-email to send the patch.

Hi,

Am 07.11.2014 um 11:14 schrieb Guo, Lei:
> This patch aims to add CMP2 instruction for m68k family.
> 
>  
> 
> *Description: *Compares the value in Rn to each bound. The effective
> address contains the
> 
> bounds pair: upper bound following the lower bound. For signed
> comparisons, the
> 
> arithmetically smaller value should be used as the lower bound. For unsigned
> 
> comparisons, the logically smaller value should be the lower bound.
> 
> The size of the data and the bounds can be specified as byte, word, or
> long. If Rn is a
> 
> data register and the operation size is byte or word, only the
> appropriate low-order part
> 
> of Rn is checked. If Rn is an address register and the operation size is
> byte or word,
> 
> the bounds operands are sign-extended to 32 bits, and the resultant
> operands are
> 
> compared to the full 32 bits of An.
> 
> If the upper bound equals the lower bound, the valid range is a single
> value.
> 
>  
> 
> signed-off-by: Guolei <guol-fnst@cn.fujitsu.com
> <mailto:guol-fnst@cn.fujitsu.com%20>>
> 
>  
> 
> ---

Please talk to your Fujitsu colleagues for how to correctly submit
patches to our mailing list. As you can see above, it arrives rather
"funny" due to your use of HTML. As a sanity check, you can mail the
patch to yourself, save it from your inbox to disk and try to apply it
via "git am yourmailfilename".

Why do you write "Description:" in the patch description? Is that a
verbatim copy from the manual? In that case you should name the source.
Otherwise please drop "Description:" and just describe it.

Also, Signed-off-by with a capital S please, and no space after your
email. If you use git commit -s you can avoid such issues from the start.

Please also use a meaningful topic in the subject, "target-m68k: Add
CMP2 instruction" tells us that this is not for x86 or ARM etc.

More information here:
http://wiki.qemu-project.org/Contribute/SubmitAPatch

As for the contents, you should be aware that QEMU currently emulates
ColdFire only and not the full 68k instruction set. So you may need to
limit your new instruction to certain CPU types. CC'ing Laurent+Andreas.

You haven't replied to Thomas' question on your previous submission
either. If you resubmit, please either mark as [PATCH RESEND], or if
things changed (such as adding the s ;)) as [PATCH v2] and include a
summary of changes below ---.

Note that there is currently no dedicated maintainer to review m68k
patches, so only obviously correct bug fixes are likely to get accepted
unless someone (you?) steps up for reviewing, testing and queuing such
patches on a Git branch.

Finally a minor nit: Please add a space between ) and {.
scripts/checkpatch.pl may help you find such style issues.
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

Looking forward to seeing the m68k target improved.

Regards,
Andreas

On 11/07/2014 11:14 AM, Guo, Lei wrote:
> This patch aims to add CMP2 instruction for m68k family.

Mainline target-m68k supports coldfire only.

There is an external tree for full m68k support:

   https://gitorious.org/qemu-m68k

That said, before you send this to them...

> +    if (ext & 0x8000) {
> +           reg = AREG(ext, 12);

Failure to sign-extend for opsize == OS_WORD.
You need to use signed comparisons for this case.

> +    } else {
> +           reg = DREG(ext, 12);
> +           if (opsize == OS_BYTE){
> +                    tcg_gen_andi_i32(reg, reg, 0xf);
> +           } else if (opsize == OS_WORD) {
> +                          tcg_gen_andi_i32(reg, reg, 0xff);
> +           }
> +    }

Incorrect zero-extension; you've messed up the constants.
Use tcg_gen_ext{8,16}u_i32, anyway.
You need to use unsigned comparisons for this case.

> +    l1 = gen_new_label();
> +    l2 = gen_new_label();
> +    l3 = gen_new_label();
> +    l4 = gen_new_label();
> +   
> +    tcg_gen_brcond_i32(TCG_COND_NE, reg, lower, l1);

Ew.  You'd be much better off doing this with setcond than brcond.

  gen_flush_flags(s);

  t1 = tcg_temp_new();
  t2 = tcg_temp_new();
  t3 = tcg_temp_new();
  t4 = tcg_temp_new();

  tcg_gen_setcond_i32(TCG_COND_EQ, t1, reg, upper);
  tcg_gen_setcond_i32(TCG_COND_EQ, t2, reg, lower);
  tcg_gen_setcond_i32(which_gt, t3, reg, upper);
  tcg_gen_setcond_i32(which_lt, t4, reg, lower);

  tcg_gen_or_i32(t1, t1, t2);		/* equal to either bound */
  tcg_gen_or_i32(t3, t3, t4);		/* out of bounds */

  tcg_gen_shl_i32(t1, t1, ctz32(CCF_Z));	/* shift Z into place */
  tcg_gen_shl_i32(t3, t3, ctz32(CCF_C));	/* shift C into place */
  tcg_gen_or_i32(t1, t1, t3);
  tcg_gen_andi_i32(QREG_CC_DEST, QREG_CC_DEST, ~(CCF_C | CCF_Z));
  tcg_gen_or_i32(QREG_CC_DEST, QREG_CC_DEST, t1);


r~

Hi,
 
did you test it ?
 
because after just a first glance this patch seems wrong as the tmp variables
cannot be used beyond the first tcg_gen_cond() (conditional branches clobber
temporary vars), you must use tcg_temp_local_new() and tcg_temp_free().

Regards,
Laurent

> Le 7 novembre 2014 à 11:14, "Guo, Lei" <guol-fnst@cn.fujitsu.com> a écrit :
> 
> 
>  This patch aims to add CMP2 instruction for m68k family.
> 
>   
> 
>  Description: Compares the value in Rn to each bound. The effective address
> contains the
> 
>  bounds pair: upper bound following the lower bound. For signed comparisons,
> the
> 
>  arithmetically smaller value should be used as the lower bound. For unsigned
> 
>  comparisons, the logically smaller value should be the lower bound.
> 
>  The size of the data and the bounds can be specified as byte, word, or long.
> If Rn is a
> 
>  data register and the operation size is byte or word, only the appropriate
> low-order part
> 
>  of Rn is checked. If Rn is an address register and the operation size is byte
> or word,
> 
>  the bounds operands are sign-extended to 32 bits, and the resultant operands
> are
> 
>  compared to the full 32 bits of An.
> 
>  If the upper bound equals the lower bound, the valid range is a single value.
> 
>   
> 
>  signed-off-by: Guolei <guol-fnst@cn.fujitsu.com
> <mailto:guol-fnst@cn.fujitsu.com%20> >
> 
>   
> 
>  ---
> 
>  target-m68k/translate.c |   74
> +++++++++++++++++++++++++++++++++++++++++++++++
> 
>  1 file changed, 74 insertions(+)
> 
>   
> 
>  diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> 
>  index efd4cfc..8c09248 100644
> 
>  --- a/target-m68k/translate.c
> 
>  +++ b/target-m68k/translate.c
> 
>  @@ -1931,6 +1931,79 @@ DISAS_INSN(ff1)
> 
>       gen_helper_ff1(reg, reg);
> 
>  }
> 
>   
> 
>  +DISAS_INSN(cmp2)
> 
>  +{
> 
>  +    TCGv src;
> 
>  +    TCGv reg;
> 
>  +    int opsize;
> 
>  +    int l1, l2, l3, l4;
> 
>  +    TCGv lower, upper;
> 
>  +    uint16_t ext;
> 
>  +   
> 
>  +    switch ((insn >> 9) & 3) {
> 
>  +           case 0:
> 
>  +                    opsize = OS_BYTE;
> 
>  +                    break;
> 
>  +           case 1:
> 
>  +                    opsize = OS_WORD;
> 
>  +                    break;
> 
>  +           case 2:
> 
>  +                    opsize = OS_LONG;
> 
>  +                    break;
> 
>  +           default:
> 
>  +                    gen_exception(s, s->pc, EXCP_ILLEGAL);
> 
>  +                    return;
> 
>  +    }
> 
>  +   
> 
>  +    SRC_EA(env, src, opsize, -1, NULL);
> 
>  +    lower = tcg_temp_new();
> 
>  +    upper = tcg_temp_new();
> 
>  +    tcg_gen_shri_i32(lower, lower, 16);
> 
>  +    tcg_gen_andi_i32(lower, lower, 0xffff);
> 
>  +    tcg_gen_andi_i32(upper, upper, 0xffff);
> 
>  +   
> 
>  +    ext = cpu_ldsw_code(env, s->pc);
> 
>  +       s->pc += 2;
> 
>  +    if (ext & 0x8000) {
> 
>  +           reg = AREG(ext, 12);
> 
>  +    } else {
> 
>  +           reg = DREG(ext, 12);
> 
>  +           if (opsize == OS_BYTE){
> 
>  +                    tcg_gen_andi_i32(reg, reg, 0xf);
> 
>  +           } else if (opsize == OS_WORD) {
> 
>  +                          tcg_gen_andi_i32(reg, reg, 0xff);
> 
>  +           }
> 
>  +    }
> 
>  +   
> 
>  +    l1 = gen_new_label();
> 
>  +    l2 = gen_new_label();
> 
>  +    l3 = gen_new_label();
> 
>  +    l4 = gen_new_label();
> 
>  +   
> 
>  +    tcg_gen_brcond_i32(TCG_COND_NE, reg, lower, l1);
> 
>  +    s->cc_op = CC_OP_FLAGS;
> 
>  +    tcg_gen_ori_i32(QREG_CC_DEST, QREG_CC_DEST, CCF_Z);
> 
>  +    tcg_gen_br(l4);
> 
>  +   
> 
>  +    gen_set_label(l1);
> 
>  +    tcg_gen_brcond_i32(TCG_COND_NE, reg, upper, l2);
> 
>  +    s->cc_op = CC_OP_FLAGS;
> 
>  +    tcg_gen_ori_i32(QREG_CC_DEST, QREG_CC_DEST, CCF_Z);
> 
>  +    tcg_gen_br(l4);
> 
>  +   
> 
>  +    gen_set_label(l2);
> 
>  +    tcg_gen_brcond_i32(TCG_COND_GT, reg, lower, l3);
> 
>  +    s->cc_op = CC_OP_FLAGS;
> 
>  +    tcg_gen_ori_i32(QREG_CC_DEST, QREG_CC_DEST, CCF_C);
> 
>  +    tcg_gen_br(l4);
> 
>  +   
> 
>  +    gen_set_label(l3);
> 
>  +    tcg_gen_brcond_i32(TCG_COND_LT, reg, upper, l4);
> 
>  +    s->cc_op = CC_OP_FLAGS;
> 
>  +    tcg_gen_ori_i32(QREG_CC_DEST, QREG_CC_DEST, CCF_C);
> 
>  +    gen_set_label(l4);
> 
>  +}
> 
>  +
> 
>  static TCGv gen_get_sr(DisasContext *s)
> 
>  {
> 
>       TCGv ccr;
> 
>  @@ -2847,6 +2920,7 @@ void register_m68k_insns (CPUM68KState *env)
> 
>       INSN(bitop_reg, 01c0, f1c0, CF_ISA_A);
> 
>       INSN(arith_im,  0280, fff8, CF_ISA_A);
> 
>       INSN(byterev,   02c0, fff8, CF_ISA_APLUSC);
> 
>  +    INSN(cmp2,      02c0, ffff, CF_ISA_A);
> 
>       INSN(arith_im,  0480, fff8, CF_ISA_A);
> 
>       INSN(ff1,       04c0, fff8, CF_ISA_APLUSC);
> 
>       INSN(arith_im,  0680, fff8, CF_ISA_A);
> 
>  --
> 
>  1.7.9.5
>

> Le 7 novembre 2014 à 11:14, "Guo, Lei" <guol-fnst@cn.fujitsu.com> a écrit :
> 
> 
>  This patch aims to add CMP2 instruction for m68k family.
> 
>   
> 
>  Description: Compares the value in Rn to each bound. The effective address
> contains the
> 
>  bounds pair: upper bound following the lower bound. For signed comparisons,
> the
> 
>  arithmetically smaller value should be used as the lower bound. For unsigned
> 
>  comparisons, the logically smaller value should be the lower bound.
> 
>  The size of the data and the bounds can be specified as byte, word, or long.
> If Rn is a
> 
>  data register and the operation size is byte or word, only the appropriate
> low-order part
> 
>  of Rn is checked. If Rn is an address register and the operation size is byte
> or word,
> 
>  the bounds operands are sign-extended to 32 bits, and the resultant operands
> are
> 
>  compared to the full 32 bits of An.
> 
>  If the upper bound equals the lower bound, the valid range is a single value.
> 

Moreover, this seems a cut&paste of the Motorola "Programmer's reference
manual".
It should be copyrighted : have we the right to copy this part in a commit
message ?

Regards,
Laurent

"Guo, Lei" <guol-fnst@cn.fujitsu.com> writes:

> @@ -2847,6 +2920,7 @@ void register_m68k_insns (CPUM68KState *env)
>      INSN(bitop_reg, 01c0, f1c0, CF_ISA_A);
>      INSN(arith_im,  0280, fff8, CF_ISA_A);
>      INSN(byterev,   02c0, fff8, CF_ISA_APLUSC);
> +    INSN(cmp2,      02c0, ffff, CF_ISA_A);

My copy of the CFPRM doesn't list cmp2 as a valid coldfire insn.

Andreas.

Hi Andreas
	Thanks a lot for your patients. Because I'm a newer to this , I'll follow your advices and pay much more attention to these details.
Besides ,I have replied to Thomas' question on my previous submission.
	Thanks again for your help.

Best regards 
Guo lei

 Re: [Qemu-devel] Add CMP2 instruction
> 

> Hi,

> 

> Am 07.11.2014 um 11:14 schrieb Guo, Lei:

> > This patch aims to add CMP2 instruction for m68k family.

> >

> >

> >

> > *Description: *Compares the value in Rn to each bound. The effective

> > address contains the

> >

> > bounds pair: upper bound following the lower bound. For signed

> > comparisons, the

> >

> > arithmetically smaller value should be used as the lower bound. For

> > unsigned

> >

> > comparisons, the logically smaller value should be the lower bound.

> >

> > The size of the data and the bounds can be specified as byte, word, or

> > long. If Rn is a

> >

> > data register and the operation size is byte or word, only the

> > appropriate low-order part

> >

> > of Rn is checked. If Rn is an address register and the operation size

> > is byte or word,

> >

> > the bounds operands are sign-extended to 32 bits, and the resultant

> > operands are

> >

> > compared to the full 32 bits of An.

> >

> > If the upper bound equals the lower bound, the valid range is a single

> > value.

> >

> >

> >

> > signed-off-by: Guolei <guol-fnst@cn.fujitsu.com

> > <mailto:guol-fnst@cn.fujitsu.com%20>>

> >

> >

> >

> > ---

> 

> Please talk to your Fujitsu colleagues for how to correctly submit patches to our

> mailing list. As you can see above, it arrives rather "funny" due to your use of

> HTML. As a sanity check, you can mail the patch to yourself, save it from your

> inbox to disk and try to apply it via "git am yourmailfilename".

> 

> Why do you write "Description:" in the patch description? Is that a verbatim

> copy from the manual? In that case you should name the source.

> Otherwise please drop "Description:" and just describe it.

> 

> Also, Signed-off-by with a capital S please, and no space after your email. If you

> use git commit -s you can avoid such issues from the start.

> 

> Please also use a meaningful topic in the subject, "target-m68k: Add

> CMP2 instruction" tells us that this is not for x86 or ARM etc.

> 

> More information here:

> http://wiki.qemu-project.org/Contribute/SubmitAPatch

> 

> As for the contents, you should be aware that QEMU currently emulates

> ColdFire only and not the full 68k instruction set. So you may need to limit your

> new instruction to certain CPU types. CC'ing Laurent+Andreas.

> 

> You haven't replied to Thomas' question on your previous submission either. If

> you resubmit, please either mark as [PATCH RESEND], or if things changed

> (such as adding the s ;)) as [PATCH v2] and include a summary of changes below

> ---.

> 

> Note that there is currently no dedicated maintainer to review m68k patches,

> so only obviously correct bug fixes are likely to get accepted unless someone

> (you?) steps up for reviewing, testing and queuing such patches on a Git

> branch.

> 

> Finally a minor nit: Please add a space between ) and {.

> scripts/checkpatch.pl may help you find such style issues.

> http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

> 

> Looking forward to seeing the m68k target improved.

> 

> Regards,

> Andreas

> 

> --

> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany

> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 21284 AG Nürnberg

Le 10/11/2014 04:21, Guo, Lei a écrit :
> Hi Andreas
> 	Thanks a lot for your patients. Because I'm a newer to this , I'll follow your advices and pay much more attention to these details.
> Besides ,I have replied to Thomas' question on my previous submission.

If the question was "Have you been in touch with the author of that
original patch already?", I can affirm the answer is "no".

It's opensoure, grab what you want, but if you resend a patch you must
let the "Signed-off-by".

BTW, "CMP2" is neither generated by gcc or used by linux, why do you
want to add it to an old and obsolete architecture ? Do you have a "real
life" test case for it ?

If you send a correct and tested new patch, I can add it in my tree.
Mainline QEMU doesn't care of m68k (680x0) support.

For those who want to know why I don't submit my patches the reasons are :
- they break coldfire support
- there is a bug in MMU emulation that prevent linux to fork new
processes (I'm hunting this for 2 years now...)

Regards,
Laurent