* [PATCH v2 0/1] tricore: fixed faulty conditions for extr and imask
@ 2021-02-11 11:53 David Brenken
2021-02-11 11:53 ` [PATCH v2 1/1] " David Brenken
0 siblings, 1 reply; 6+ messages in thread
From: David Brenken @ 2021-02-11 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kbastian, Andreas Konopik
From: Andreas Konopik <andreas.konopik@efs-auto.de>
Hello together,
we have fixed a few conditions leading to incorrect intermediate code
generation.
Andreas Konopik (1):
tricore: fixed faulty conditions for extr and imask
target/tricore/translate.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--
2.30.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/1] tricore: fixed faulty conditions for extr and imask
2021-02-11 11:53 [PATCH v2 0/1] tricore: fixed faulty conditions for extr and imask David Brenken
@ 2021-02-11 11:53 ` David Brenken
2021-02-11 12:10 ` Philippe Mathieu-Daudé
2021-03-05 11:50 ` Bastian Koppelmann
0 siblings, 2 replies; 6+ messages in thread
From: David Brenken @ 2021-02-11 11:53 UTC (permalink / raw)
To: qemu-devel; +Cc: kbastian, David Brenken, Georg Hofstetter, Andreas Konopik
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(-)
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);
}
--
2.30.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] tricore: fixed faulty conditions for extr and imask
2021-02-11 11:53 ` [PATCH v2 1/1] " David Brenken
@ 2021-02-11 12:10 ` Philippe Mathieu-Daudé
2021-02-11 13:49 ` Konopik, Andreas (EFS-GH2)
2021-03-05 11:50 ` Bastian Koppelmann
1 sibling, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-11 12:10 UTC (permalink / raw)
To: David Brenken, qemu-devel
Cc: kbastian, David Brenken, Georg Hofstetter, Andreas Konopik
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);
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2 1/1] tricore: fixed faulty conditions for extr and imask
2021-02-11 12:10 ` Philippe Mathieu-Daudé
@ 2021-02-11 13:49 ` Konopik, Andreas (EFS-GH2)
2021-03-05 12:43 ` Bastian Koppelmann
0 siblings, 1 reply; 6+ messages in thread
From: Konopik, Andreas (EFS-GH2) @ 2021-02-11 13:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, David Brenken, qemu-devel
Cc: kbastian, Brenken, David (EFS-GH5), Hofstetter, Georg (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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] tricore: fixed faulty conditions for extr and imask
2021-02-11 11:53 ` [PATCH v2 1/1] " David Brenken
2021-02-11 12:10 ` Philippe Mathieu-Daudé
@ 2021-03-05 11:50 ` Bastian Koppelmann
1 sibling, 0 replies; 6+ messages in thread
From: Bastian Koppelmann @ 2021-03-05 11:50 UTC (permalink / raw)
To: David Brenken
Cc: Andreas Konopik, David Brenken, qemu-devel, Georg Hofstetter
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] tricore: fixed faulty conditions for extr and imask
2021-02-11 13:49 ` Konopik, Andreas (EFS-GH2)
@ 2021-03-05 12:43 ` Bastian Koppelmann
0 siblings, 0 replies; 6+ messages in thread
From: Bastian Koppelmann @ 2021-03-05 12:43 UTC (permalink / raw)
To: Konopik, Andreas (EFS-GH2)
Cc: qemu-devel, Hofstetter, Georg (EFS-GH2), Brenken, David (EFS-GH5),
Philippe Mathieu-Daudé,
David Brenken
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-05 12:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11 11:53 [PATCH v2 0/1] tricore: fixed faulty conditions for extr and imask David Brenken
2021-02-11 11:53 ` [PATCH v2 1/1] " David Brenken
2021-02-11 12:10 ` Philippe Mathieu-Daudé
2021-02-11 13:49 ` Konopik, Andreas (EFS-GH2)
2021-03-05 12:43 ` Bastian Koppelmann
2021-03-05 11:50 ` Bastian Koppelmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.