* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
@ 2013-09-05 12:00 Jay Foad
2013-09-05 20:17 ` Stefan Weil
0 siblings, 1 reply; 9+ messages in thread
From: Jay Foad @ 2013-09-05 12:00 UTC (permalink / raw)
To: Stefan Weil; +Cc: qemu-devel
> diff --git a/tci.c b/tci.c
> index 18c888e..94b7851 100644
> --- a/tci.c
> +++ b/tci.c
> @@ -952,8 +952,16 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
> break;
> #if TCG_TARGET_HAS_rot_i64
> case INDEX_op_rotl_i64:
> + t0 = *tb_ptr++;
> + t1 = tci_read_ri64(&tb_ptr);
> + t2 = tci_read_ri64(&tb_ptr);
> + tci_write_reg64(t0, (t1 << t2) | (t1 >> (64 - t2)));
> + break;
> case INDEX_op_rotr_i64:
> - TODO();
> + t0 = *tb_ptr++;
> + t1 = tci_read_ri64(&tb_ptr);
> + t2 = tci_read_ri64(&tb_ptr);
> + tci_write_reg64(t0, (t1 >> t2) | (t1 << (64 - t2)));
<< (64 - t2) is undefined behaviour in C when t2 is 0. How about << (-t2 & 63) ?
Jay.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-05 12:00 [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64 Jay Foad
@ 2013-09-05 20:17 ` Stefan Weil
2013-09-05 20:33 ` Richard Henderson
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Weil @ 2013-09-05 20:17 UTC (permalink / raw)
To: Jay Foad; +Cc: qemu-devel, Richard Henderson
Am 05.09.2013 14:00, schrieb Jay Foad:
>> diff --git a/tci.c b/tci.c
>> index 18c888e..94b7851 100644
>> --- a/tci.c
>> +++ b/tci.c
>> @@ -952,8 +952,16 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
>> break;
>> #if TCG_TARGET_HAS_rot_i64
>> case INDEX_op_rotl_i64:
>> + t0 = *tb_ptr++;
>> + t1 = tci_read_ri64(&tb_ptr);
>> + t2 = tci_read_ri64(&tb_ptr);
>> + tci_write_reg64(t0, (t1 << t2) | (t1 >> (64 - t2)));
>> + break;
>> case INDEX_op_rotr_i64:
>> - TODO();
>> + t0 = *tb_ptr++;
>> + t1 = tci_read_ri64(&tb_ptr);
>> + t2 = tci_read_ri64(&tb_ptr);
>> + tci_write_reg64(t0, (t1 >> t2) | (t1 << (64 - t2)));
> << (64 - t2) is undefined behaviour in C when t2 is 0. How about << (-t2 & 63) ?
>
> Jay.
A short test confirms that the behaviour for (t1 << 64) is indeed
unexpected.
I added assertions for (t2 > 0) and (t2 < 64). They never raised an abort.
Are those cases possible? We already have similar code for 32 bit shifts,
and tcg/optimize.c also includes an implementation which is identical to
my rotl_i64, rotr_i64.
Therefore I think my patch can be applied as it is.
Stefan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-05 20:17 ` Stefan Weil
@ 2013-09-05 20:33 ` Richard Henderson
0 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2013-09-05 20:33 UTC (permalink / raw)
To: Stefan Weil; +Cc: Jay Foad, qemu-devel
On 09/05/2013 01:17 PM, Stefan Weil wrote:
> I added assertions for (t2 > 0) and (t2 < 64). They never raised an abort.
> Are those cases possible? We already have similar code for 32 bit shifts,
> and tcg/optimize.c also includes an implementation which is identical to
> my rotl_i64, rotr_i64.
It is possible, but very rare. In the tcg optimizer, by looking through
constants that the tcg-op.h routines couldn't filter. In the tci backend
by seeing the real data that wasn't constant for the optimizer.
We should handle zero and modulo count rotates in both places.
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
@ 2013-09-04 20:49 Stefan Weil
2013-09-04 20:51 ` Stefan Weil
2013-09-04 23:00 ` Richard Henderson
0 siblings, 2 replies; 9+ messages in thread
From: Stefan Weil @ 2013-09-04 20:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Weil, Richard Henderson
It is used by qemu-ppc64 when running Debian's busybox-static.
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
This patch is also useful for QEMU 1.6.
tcg/tci/tcg-target.c | 1 -
tci.c | 10 +++++++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
index 233ab3b..4976bec 100644
--- a/tcg/tci/tcg-target.c
+++ b/tcg/tci/tcg-target.c
@@ -670,7 +670,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
case INDEX_op_shl_i64:
case INDEX_op_shr_i64:
case INDEX_op_sar_i64:
- /* TODO: Implementation of rotl_i64, rotr_i64 missing in tci.c. */
case INDEX_op_rotl_i64: /* Optional (TCG_TARGET_HAS_rot_i64). */
case INDEX_op_rotr_i64: /* Optional (TCG_TARGET_HAS_rot_i64). */
tcg_out_r(s, args[0]);
diff --git a/tci.c b/tci.c
index 18c888e..94b7851 100644
--- a/tci.c
+++ b/tci.c
@@ -952,8 +952,16 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
break;
#if TCG_TARGET_HAS_rot_i64
case INDEX_op_rotl_i64:
+ t0 = *tb_ptr++;
+ t1 = tci_read_ri64(&tb_ptr);
+ t2 = tci_read_ri64(&tb_ptr);
+ tci_write_reg64(t0, (t1 << t2) | (t1 >> (64 - t2)));
+ break;
case INDEX_op_rotr_i64:
- TODO();
+ t0 = *tb_ptr++;
+ t1 = tci_read_ri64(&tb_ptr);
+ t2 = tci_read_ri64(&tb_ptr);
+ tci_write_reg64(t0, (t1 >> t2) | (t1 << (64 - t2)));
break;
#endif
#if TCG_TARGET_HAS_deposit_i64
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-04 20:49 Stefan Weil
@ 2013-09-04 20:51 ` Stefan Weil
2013-09-09 14:53 ` Aurelien Jarno
2013-09-04 23:00 ` Richard Henderson
1 sibling, 1 reply; 9+ messages in thread
From: Stefan Weil @ 2013-09-04 20:51 UTC (permalink / raw)
To: qemu-stable; +Cc: qemu-devel, Richard Henderson
Am 04.09.2013 22:49, schrieb Stefan Weil:
> It is used by qemu-ppc64 when running Debian's busybox-static.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> This patch is also useful for QEMU 1.6.
... so I should have added qemu-stable. Done now.
>
> tcg/tci/tcg-target.c | 1 -
> tci.c | 10 +++++++++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tcg/tci/tcg-target.c b/tcg/tci/tcg-target.c
> index 233ab3b..4976bec 100644
> --- a/tcg/tci/tcg-target.c
> +++ b/tcg/tci/tcg-target.c
> @@ -670,7 +670,6 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
> case INDEX_op_shl_i64:
> case INDEX_op_shr_i64:
> case INDEX_op_sar_i64:
> - /* TODO: Implementation of rotl_i64, rotr_i64 missing in tci.c. */
> case INDEX_op_rotl_i64: /* Optional (TCG_TARGET_HAS_rot_i64). */
> case INDEX_op_rotr_i64: /* Optional (TCG_TARGET_HAS_rot_i64). */
> tcg_out_r(s, args[0]);
> diff --git a/tci.c b/tci.c
> index 18c888e..94b7851 100644
> --- a/tci.c
> +++ b/tci.c
> @@ -952,8 +952,16 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
> break;
> #if TCG_TARGET_HAS_rot_i64
> case INDEX_op_rotl_i64:
> + t0 = *tb_ptr++;
> + t1 = tci_read_ri64(&tb_ptr);
> + t2 = tci_read_ri64(&tb_ptr);
> + tci_write_reg64(t0, (t1 << t2) | (t1 >> (64 - t2)));
> + break;
> case INDEX_op_rotr_i64:
> - TODO();
> + t0 = *tb_ptr++;
> + t1 = tci_read_ri64(&tb_ptr);
> + t2 = tci_read_ri64(&tb_ptr);
> + tci_write_reg64(t0, (t1 >> t2) | (t1 << (64 - t2)));
> break;
> #endif
> #if TCG_TARGET_HAS_deposit_i64
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-04 20:51 ` Stefan Weil
@ 2013-09-09 14:53 ` Aurelien Jarno
2013-09-09 16:14 ` Stefan Weil
0 siblings, 1 reply; 9+ messages in thread
From: Aurelien Jarno @ 2013-09-09 14:53 UTC (permalink / raw)
To: Stefan Weil; +Cc: Richard Henderson, qemu-stable, qemu-devel
On Wed, Sep 04, 2013 at 10:51:54PM +0200, Stefan Weil wrote:
> Am 04.09.2013 22:49, schrieb Stefan Weil:
> > It is used by qemu-ppc64 when running Debian's busybox-static.
> >
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> >
> > This patch is also useful for QEMU 1.6.
>
> ... so I should have added qemu-stable. Done now.
>
Why is it needed for qemu-stable? It should only improve performance, as
these two ops are not mandatory ops. In general I don't think we should
have performance improvements in a stable minor release (unless of course
it's a big regression from previous).
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-09 14:53 ` Aurelien Jarno
@ 2013-09-09 16:14 ` Stefan Weil
2013-09-09 16:47 ` Aurelien Jarno
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Weil @ 2013-09-09 16:14 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: Richard Henderson, qemu-stable, qemu-devel
Am 09.09.2013 16:53, schrieb Aurelien Jarno:
> On Wed, Sep 04, 2013 at 10:51:54PM +0200, Stefan Weil wrote:
>> Am 04.09.2013 22:49, schrieb Stefan Weil:
>>> It is used by qemu-ppc64 when running Debian's busybox-static.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>
>>> This patch is also useful for QEMU 1.6.
>> ... so I should have added qemu-stable. Done now.
>>
> Why is it needed for qemu-stable? It should only improve performance, as
> these two ops are not mandatory ops. In general I don't think we should
> have performance improvements in a stable minor release (unless of course
> it's a big regression from previous).
Hi Aurelien,
tci has always set TCG_TARGET_HAS_rot_i64, and target-ppc64 uses the
rotl_i64 and rotr_i64 TCG opcodes.
The old implementation just triggered a TODO assertion. That's how I
implemented TCI: I started with TODO assertions for all opcodes and
replaced them by real code as soon as I got a test case which used them.
Now I have a test case :-)
=> It's not a performance issue, but fixes a fatal assertion.
=> It should be used in 1.6.
Regards,
Stefan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-09 16:14 ` Stefan Weil
@ 2013-09-09 16:47 ` Aurelien Jarno
0 siblings, 0 replies; 9+ messages in thread
From: Aurelien Jarno @ 2013-09-09 16:47 UTC (permalink / raw)
To: Stefan Weil; +Cc: Richard Henderson, qemu-stable, qemu-devel
On Mon, Sep 09, 2013 at 06:14:34PM +0200, Stefan Weil wrote:
> Am 09.09.2013 16:53, schrieb Aurelien Jarno:
> > On Wed, Sep 04, 2013 at 10:51:54PM +0200, Stefan Weil wrote:
> >> Am 04.09.2013 22:49, schrieb Stefan Weil:
> >>> It is used by qemu-ppc64 when running Debian's busybox-static.
> >>>
> >>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>> ---
> >>>
> >>> This patch is also useful for QEMU 1.6.
> >> ... so I should have added qemu-stable. Done now.
> >>
> > Why is it needed for qemu-stable? It should only improve performance, as
> > these two ops are not mandatory ops. In general I don't think we should
> > have performance improvements in a stable minor release (unless of course
> > it's a big regression from previous).
>
> Hi Aurelien,
>
> tci has always set TCG_TARGET_HAS_rot_i64, and target-ppc64 uses the
> rotl_i64 and rotr_i64 TCG opcodes.
>
> The old implementation just triggered a TODO assertion. That's how I
> implemented TCI: I started with TODO assertions for all opcodes and
> replaced them by real code as soon as I got a test case which used them.
> Now I have a test case :-)
Ok, so it might be a good idea to review the remaining non-implemented
optional ops and to disable them to avoid that in the future.
> => It's not a performance issue, but fixes a fatal assertion.
> => It should be used in 1.6.
>
It should probably also be used in older versions, as rotl and rotr have
been used by various targets for a few years (at least since version
0.10.0, probably even before).
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64
2013-09-04 20:49 Stefan Weil
2013-09-04 20:51 ` Stefan Weil
@ 2013-09-04 23:00 ` Richard Henderson
1 sibling, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2013-09-04 23:00 UTC (permalink / raw)
To: Stefan Weil; +Cc: qemu-devel
On 09/04/2013 01:49 PM, Stefan Weil wrote:
> It is used by qemu-ppc64 when running Debian's busybox-static.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> This patch is also useful for QEMU 1.6.
Reviewed-by: Richard Henderson <rth@twiddle.net>
r~
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-09-09 16:48 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-05 12:00 [Qemu-devel] [PATCH] tci: Add implementation of rotl_i64, rotr_i64 Jay Foad
2013-09-05 20:17 ` Stefan Weil
2013-09-05 20:33 ` Richard Henderson
-- strict thread matches above, loose matches on Subject: below --
2013-09-04 20:49 Stefan Weil
2013-09-04 20:51 ` Stefan Weil
2013-09-09 14:53 ` Aurelien Jarno
2013-09-09 16:14 ` Stefan Weil
2013-09-09 16:47 ` Aurelien Jarno
2013-09-04 23:00 ` Richard Henderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).