Message ID | 20210211115329.8984-2-david.brenken@efs-auto.org |
---|---|
State | New |
Headers | show |
Series | tricore: fixed faulty conditions for extr and imask | expand |
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 > > 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); > } >
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
On Thu, Feb 11, 2021 at 12:53:29PM +0100, David Brenken wrote: > From: Andreas Konopik <andreas.konopik@efs-auto.de> > > 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(-) Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> Applied to my tricore.next queue. I'll fix the commit message according to Philippes suggestions. Cheers, Bastian
On Thu, Feb 11, 2021 at 01:49:14PM +0000, Konopik, Andreas (EFS-GH2) wrote: > 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. While editing the commit message of this patch, I realized that we are not checking for width=0. A quick test let to: qemu-system-tricore: ../upstream/tcg/tcg-op.c:217: tcg_gen_sari_i32: Assertion `arg2 >= 0 && arg2 < 32' failed. As Richard suggested that is not what should happen in case of undefined behaviour. I'll fix this in a another patch. Cheers, Bastian
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); }