1. Patchew
  2. QEMU
  3. View series

[PATCH v2] target/ppc: Fix for optimized vsl/vsr instructions

Stefan Brankovic posted 1 patch 6 years, 1 month ago
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu failed
Test asan passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1570196639-7025-2-git-send-email-stefan.brankovic@rt-rk.com
Maintainers: David Gibson <david@gibson.dropbear.id.au>
target/ppc/translate/vmx-impl.inc.c | 84 ++++++++++++++++++-------------------
1 file changed, 40 insertions(+), 44 deletions(-)
[PATCH v2] target/ppc: Fix for optimized vsl/vsr instructions
Posted by Stefan Brankovic 6 years, 1 month ago 
In previous implementation, invocation of TCG shift function could request
shift of TCG variable by 64 bits when variable 'sh' is 0, which is not
supported in TCG (values can be shifted by 0 to 63 bits). This patch fixes
this by using two separate invocation of TCG shift functions, with maximum
shift amount of 32.

Name of variable 'shifted' is changed to 'carry' so variable naming
is similar to old helper implementation.

Variables 'avrA' and 'avrB' are replaced with variable 'avr'.

Fixes: 4e6d0920e7547e6af4bbac5ffe9adfe6ea621822
Reported-by: Paul Clark <pc@us.ibm.com>
Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Suggested-by: Aleksandar Markovic <aleksandar.markovic@rt-rk.com>
Signed-off-by: Stefan Brankovic <stefan.brankovic@rt-rk.com>
---
 target/ppc/translate/vmx-impl.inc.c | 84 ++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 44 deletions(-)

diff --git a/target/ppc/translate/vmx-impl.inc.c b/target/ppc/translate/vmx-impl.inc.c
index 2472a52..81d5a7a 100644
--- a/target/ppc/translate/vmx-impl.inc.c
+++ b/target/ppc/translate/vmx-impl.inc.c
@@ -590,40 +590,38 @@ static void trans_vsl(DisasContext *ctx)
     int VT = rD(ctx->opcode);
     int VA = rA(ctx->opcode);
     int VB = rB(ctx->opcode);
-    TCGv_i64 avrA = tcg_temp_new_i64();
-    TCGv_i64 avrB = tcg_temp_new_i64();
+    TCGv_i64 avr = tcg_temp_new_i64();
     TCGv_i64 sh = tcg_temp_new_i64();
-    TCGv_i64 shifted = tcg_temp_new_i64();
+    TCGv_i64 carry = tcg_temp_new_i64();
     TCGv_i64 tmp = tcg_temp_new_i64();
 
-    /* Place bits 125-127 of vB in sh. */
-    get_avr64(avrB, VB, false);
-    tcg_gen_andi_i64(sh, avrB, 0x07ULL);
+    /* Place bits 125-127 of vB in 'sh'. */
+    get_avr64(avr, VB, false);
+    tcg_gen_andi_i64(sh, avr, 0x07ULL);
 
     /*
-     * Save highest sh bits of lower doubleword element of vA in variable
-     * shifted and perform shift on lower doubleword.
+     * Save highest 'sh' bits of lower doubleword element of vA in variable
+     * 'carry' and perform shift on lower doubleword.
      */
-    get_avr64(avrA, VA, false);
-    tcg_gen_subfi_i64(tmp, 64, sh);
-    tcg_gen_shr_i64(shifted, avrA, tmp);
-    tcg_gen_andi_i64(shifted, shifted, 0x7fULL);
-    tcg_gen_shl_i64(avrA, avrA, sh);
-    set_avr64(VT, avrA, false);
+    get_avr64(avr, VA, false);
+    tcg_gen_subfi_i64(tmp, 32, sh);
+    tcg_gen_shri_i64(carry, avr, 32);
+    tcg_gen_shr_i64(carry, carry, tmp);
+    tcg_gen_shl_i64(avr, avr, sh);
+    set_avr64(VT, avr, false);
 
     /*
      * Perform shift on higher doubleword element of vA and replace lowest
-     * sh bits with shifted.
+     * 'sh' bits with 'carry'.
      */
-    get_avr64(avrA, VA, true);
-    tcg_gen_shl_i64(avrA, avrA, sh);
-    tcg_gen_or_i64(avrA, avrA, shifted);
-    set_avr64(VT, avrA, true);
+    get_avr64(avr, VA, true);
+    tcg_gen_shl_i64(avr, avr, sh);
+    tcg_gen_or_i64(avr, avr, carry);
+    set_avr64(VT, avr, true);
 
-    tcg_temp_free_i64(avrA);
-    tcg_temp_free_i64(avrB);
+    tcg_temp_free_i64(avr);
     tcg_temp_free_i64(sh);
-    tcg_temp_free_i64(shifted);
+    tcg_temp_free_i64(carry);
     tcg_temp_free_i64(tmp);
 }
 
@@ -639,39 +637,37 @@ static void trans_vsr(DisasContext *ctx)
     int VT = rD(ctx->opcode);
     int VA = rA(ctx->opcode);
     int VB = rB(ctx->opcode);
-    TCGv_i64 avrA = tcg_temp_new_i64();
-    TCGv_i64 avrB = tcg_temp_new_i64();
+    TCGv_i64 avr = tcg_temp_new_i64();
     TCGv_i64 sh = tcg_temp_new_i64();
-    TCGv_i64 shifted = tcg_temp_new_i64();
+    TCGv_i64 carry = tcg_temp_new_i64();
     TCGv_i64 tmp = tcg_temp_new_i64();
 
-    /* Place bits 125-127 of vB in sh. */
-    get_avr64(avrB, VB, false);
-    tcg_gen_andi_i64(sh, avrB, 0x07ULL);
+    /* Place bits 125-127 of vB in 'sh'. */
+    get_avr64(avr, VB, false);
+    tcg_gen_andi_i64(sh, avr, 0x07ULL);
 
     /*
-     * Save lowest sh bits of higher doubleword element of vA in variable
-     * shifted and perform shift on higher doubleword.
+     * Save lowest 'sh' bits of higher doubleword element of vA in variable
+     * 'carry' and perform shift on higher doubleword.
      */
-    get_avr64(avrA, VA, true);
-    tcg_gen_subfi_i64(tmp, 64, sh);
-    tcg_gen_shl_i64(shifted, avrA, tmp);
-    tcg_gen_andi_i64(shifted, shifted, 0xfe00000000000000ULL);
-    tcg_gen_shr_i64(avrA, avrA, sh);
-    set_avr64(VT, avrA, true);
+    get_avr64(avr, VA, true);
+    tcg_gen_subfi_i64(tmp, 32, sh);
+    tcg_gen_shli_i64(carry, avr, 32);
+    tcg_gen_shl_i64(carry, carry, tmp);
+    tcg_gen_shr_i64(avr, avr, sh);
+    set_avr64(VT, avr, true);
     /*
      * Perform shift on lower doubleword element of vA and replace highest
-     * sh bits with shifted.
+     * 'sh' bits with 'carry'.
      */
-    get_avr64(avrA, VA, false);
-    tcg_gen_shr_i64(avrA, avrA, sh);
-    tcg_gen_or_i64(avrA, avrA, shifted);
-    set_avr64(VT, avrA, false);
+    get_avr64(avr, VA, false);
+    tcg_gen_shr_i64(avr, avr, sh);
+    tcg_gen_or_i64(avr, avr, carry);
+    set_avr64(VT, avr, false);
 
-    tcg_temp_free_i64(avrA);
-    tcg_temp_free_i64(avrB);
+    tcg_temp_free_i64(avr);
     tcg_temp_free_i64(sh);
-    tcg_temp_free_i64(shifted);
+    tcg_temp_free_i64(carry);
     tcg_temp_free_i64(tmp);
 }
 
-- 
2.7.4
Re: [PATCH v2] target/ppc: Fix for optimized vsl/vsr instructions
Posted by Aleksandar Markovic 6 years, 1 month ago 
> Reported-by: Paul Clark <pc@us.ibm.com>

Stefan,

Paul's full name is Paul A. Clarke.

Thanks for the fix!

Aleksandar
Re: [PATCH v2] target/ppc: Fix for optimized vsl/vsr instructions
Posted by Paul Clarke 6 years, 1 month ago 
On 10/4/19 8:43 AM, Stefan Brankovic wrote:
> In previous implementation, invocation of TCG shift function could request
> shift of TCG variable by 64 bits when variable 'sh' is 0, which is not
> supported in TCG (values can be shifted by 0 to 63 bits). This patch fixes
> this by using two separate invocation of TCG shift functions, with maximum
> shift amount of 32.
> 
> Name of variable 'shifted' is changed to 'carry' so variable naming
> is similar to old helper implementation.
> 
> Variables 'avrA' and 'avrB' are replaced with variable 'avr'.
> 
> Fixes: 4e6d0920e7547e6af4bbac5ffe9adfe6ea621822
> Reported-by: Paul Clark <pc@us.ibm.com>

Preferred: "Paul A. Clarke" (for historical consistency)

> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Suggested-by: Aleksandar Markovic <aleksandar.markovic@rt-rk.com>
> Signed-off-by: Stefan Brankovic <stefan.brankovic@rt-rk.com>

Applying this patch on top of dce5a787c05fe1a3e54d92871cdeba2af6798e0d eliminated the failures that I reported in https://bugs.launchpad.net/qemu/+bug/1841990 associated with vsl/vsr.

Tested-by: Paul A. Clarke  <pc@us.ibm.com>

PC
Re: [PATCH v2] target/ppc: Fix for optimized vsl/vsr instructions
Posted by David Gibson 6 years, 1 month ago 
On Fri, Oct 04, 2019 at 01:52:41PM -0500, Paul Clarke wrote:
> On 10/4/19 8:43 AM, Stefan Brankovic wrote:
> > In previous implementation, invocation of TCG shift function could request
> > shift of TCG variable by 64 bits when variable 'sh' is 0, which is not
> > supported in TCG (values can be shifted by 0 to 63 bits). This patch fixes
> > this by using two separate invocation of TCG shift functions, with maximum
> > shift amount of 32.
> > 
> > Name of variable 'shifted' is changed to 'carry' so variable naming
> > is similar to old helper implementation.
> > 
> > Variables 'avrA' and 'avrB' are replaced with variable 'avr'.
> > 
> > Fixes: 4e6d0920e7547e6af4bbac5ffe9adfe6ea621822
> > Reported-by: Paul Clark <pc@us.ibm.com>
> 
> Preferred: "Paul A. Clarke" (for historical consistency)
> 
> > Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Suggested-by: Aleksandar Markovic <aleksandar.markovic@rt-rk.com>
> > Signed-off-by: Stefan Brankovic <stefan.brankovic@rt-rk.com>
> 
> Applying this patch on top of dce5a787c05fe1a3e54d92871cdeba2af6798e0d eliminated the failures that I reported in https://bugs.launchpad.net/qemu/+bug/1841990 associated with vsl/vsr.
> 
> Tested-by: Paul A. Clarke  <pc@us.ibm.com>

I've applied this to ppc-for-4.2, adjusting the Reported-by line as
suggested above.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.