RE: [PATCH v2 1/1] tricore: fixed faulty conditions for extr and imask

[Konopik, Andreas (EFS-GH2)]

Hi Philippe,

> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On
> Behalf Of Philippe Mathieu-Daudé
> Sent: Thursday, February 11, 2021 13:10
> Hi David and Andreas,
> 
> On 2/11/21 12:53 PM, David Brenken wrote:
> > From: Andreas Konopik <andreas.konopik@efs-auto.de>
> 
> Here is a good place to explain why you need this change, how did you
> noticed it (example of opcode and conditions reaching this issue) - eventually
> provide a reproducer (asm dump could be enough) - and also eventually a
> reference to the manual (chapter, table) justifying your change.
> 
> See also:
> https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
> https://chris.beams.io/posts/git-commit/#why-not-how

I appreciate your feedback and will write more verbose commit messages in
the future.

According to the TC 1.3.1. Architecture Manual [1; page 174], results are
undefined, if pos + width > 32 or if width = 0.

We found this error because of a different behavior between qemu-tricore
and the real tricore processor. For pos + width = 32, qemu-tricore did not
generate any intermediate code and ran into a different state compared to
the real hardware.

I hope this helps!

> >
> > Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> > Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> > Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> > ---
> >  target/tricore/translate.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> > index 7752630ac1..ebeddf8f4a 100644
> > --- a/target/tricore/translate.c
> > +++ b/target/tricore/translate.c
> > @@ -5777,8 +5777,8 @@ static void decode_rcpw_insert(DisasContext
> *ctx)
> >      switch (op2) {
> >      case OPC2_32_RCPW_IMASK:
> >          CHECK_REG_PAIR(r2);
> > -        /* if pos + width > 31 undefined result */
> > -        if (pos + width <= 31) {
> > +        /* if pos + width > 32 undefined result */
> > +        if (pos + width <= 32) {
> >              tcg_gen_movi_tl(cpu_gpr_d[r2+1], ((1u << width) - 1) << pos);
> >              tcg_gen_movi_tl(cpu_gpr_d[r2], (const4 << pos));
> >          }
> > @@ -6999,7 +6999,7 @@ static void
> > decode_rrpw_extract_insert(DisasContext *ctx)
> >
> >      switch (op2) {
> >      case OPC2_32_RRPW_EXTR:
> > -        if (pos + width <= 31) {
> > +        if (pos + width <= 32) {
> >              /* optimize special cases */
> >              if ((pos == 0) && (width == 8)) {
> >                  tcg_gen_ext8s_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]); @@
> > -7021,7 +7021,7 @@ static void decode_rrpw_extract_insert(DisasContext
> *ctx)
> >          break;
> >      case OPC2_32_RRPW_IMASK:
> >          CHECK_REG_PAIR(r3);
> > -        if (pos + width <= 31) {
> > +        if (pos + width <= 32) {
> >              tcg_gen_movi_tl(cpu_gpr_d[r3+1], ((1u << width) - 1) << pos);
> >              tcg_gen_shli_tl(cpu_gpr_d[r3], cpu_gpr_d[r2], pos);
> >          }
> >

[1] 
https://www.infineon.com/dgdl/tc_v131_instructionset_v138.pdf?fileId=db3a304412b407950112b409b6dd0352

INTERNAL