[PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Peter Bergner]

The xxspltib instruction splats the 8-bit IMM8 operand into each byte of
the VSX register.  We currently only accept unsigned values that fit in a
byte.  With this change, we now accept signed or unsigned values that fit
within a byte.

Committed.

Peter


opcodes/
	* ppc-opc.c (IMM8): Use PPC_OPERAND_SIGNOPT.

gas/
	* testsuite/gas/ppc/power9.d <xxspltib>: Add additional operand tests.
	* testsuite/gas/ppc/power9.s: Likewise.

diff --git a/opcodes/ppc-opc.c b/opcodes/ppc-opc.c
index d4ba5d4..04f2231 100644
--- a/opcodes/ppc-opc.c
+++ b/opcodes/ppc-opc.c
@@ -942,7 +942,7 @@ const struct powerpc_operand powerpc_operands[] =

   /* The 8-bit IMM8 field in a XX1 form instruction.  */
 #define IMM8 IH + 1
-  { 0xff, 11, NULL, NULL, 0 },
+  { 0xff, 11, NULL, NULL, PPC_OPERAND_SIGNOPT },
 };

 const unsigned int num_powerpc_operands = (sizeof (powerpc_operands)
diff --git a/gas/testsuite/gas/ppc/power9.d b/gas/testsuite/gas/ppc/power9.d
index 2e5593d..d84e635 100644
--- a/gas/testsuite/gas/ppc/power9.d
+++ b/gas/testsuite/gas/ppc/power9.d
@@ -140,6 +140,8 @@ Disassembly of section \.text:
 .*:	(f0 80 2a 94|94 2a 80 f0) 	xxextractuw vs4,vs5,0
 .*:	(f1 0f 92 97|97 92 0f f1) 	xxextractuw vs40,vs50,15
 .*:	(f0 80 02 d0|d0 02 80 f0) 	xxspltib vs4,0
+.*:	(f0 80 02 d0|d0 02 80 f0) 	xxspltib vs4,0
+.*:	(f1 27 fa d1|d1 fa 27 f1) 	xxspltib vs41,255
 .*:	(f1 27 fa d1|d1 fa 27 f1) 	xxspltib vs41,255
 .*:	(f0 a0 32 d4|d4 32 a0 f0) 	xxinsertw vs5,vs6,0
 .*:	(f2 4f e2 d7|d7 e2 4f f2) 	xxinsertw vs50,vs60,15
diff --git a/gas/testsuite/gas/ppc/power9.s b/gas/testsuite/gas/ppc/power9.s
index 6ee49d4..9cf306c 100644
--- a/gas/testsuite/gas/ppc/power9.s
+++ b/gas/testsuite/gas/ppc/power9.s
@@ -131,7 +131,9 @@ power9:
 	xxextractuw 4,5,0x0
 	xxextractuw 40,50,0xf
 	xxspltib    4,0x0
+	xxspltib    4,-256
 	xxspltib    41,255
+	xxspltib    41,-1
 	xxinsertw   5,6,0
 	xxinsertw   50,60,0xf
 	xxbrh       6,7

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Jan Beulich]

>>> On 13.05.16 at 22:57, <bergner@vnet.ibm.com> wrote:
> The xxspltib instruction splats the 8-bit IMM8 operand into each byte of
> the VSX register.  We currently only accept unsigned values that fit in a
> byte.  With this change, we now accept signed or unsigned values that fit
> within a byte.
> 
> Committed.
> 
> Peter
> 
> 
> opcodes/
> 	* ppc-opc.c (IMM8): Use PPC_OPERAND_SIGNOPT.
> 
> gas/
> 	* testsuite/gas/ppc/power9.d <xxspltib>: Add additional operand tests.
> 	* testsuite/gas/ppc/power9.s: Likewise.
> 
> diff --git a/opcodes/ppc-opc.c b/opcodes/ppc-opc.c
> index d4ba5d4..04f2231 100644
> --- a/opcodes/ppc-opc.c
> +++ b/opcodes/ppc-opc.c
> @@ -942,7 +942,7 @@ const struct powerpc_operand powerpc_operands[] =
> 
>    /* The 8-bit IMM8 field in a XX1 form instruction.  */
>  #define IMM8 IH + 1
> -  { 0xff, 11, NULL, NULL, 0 },
> +  { 0xff, 11, NULL, NULL, PPC_OPERAND_SIGNOPT },
>  };
> 
>  const unsigned int num_powerpc_operands = (sizeof (powerpc_operands)
> diff --git a/gas/testsuite/gas/ppc/power9.d b/gas/testsuite/gas/ppc/power9.d
> index 2e5593d..d84e635 100644
> --- a/gas/testsuite/gas/ppc/power9.d
> +++ b/gas/testsuite/gas/ppc/power9.d
> @@ -140,6 +140,8 @@ Disassembly of section \.text:
>  .*:	(f0 80 2a 94|94 2a 80 f0) 	xxextractuw vs4,vs5,0
>  .*:	(f1 0f 92 97|97 92 0f f1) 	xxextractuw vs40,vs50,15
>  .*:	(f0 80 02 d0|d0 02 80 f0) 	xxspltib vs4,0
> +.*:	(f0 80 02 d0|d0 02 80 f0) 	xxspltib vs4,0
> +.*:	(f1 27 fa d1|d1 fa 27 f1) 	xxspltib vs41,255
>  .*:	(f1 27 fa d1|d1 fa 27 f1) 	xxspltib vs41,255
>  .*:	(f0 a0 32 d4|d4 32 a0 f0) 	xxinsertw vs5,vs6,0
>  .*:	(f2 4f e2 d7|d7 e2 4f f2) 	xxinsertw vs50,vs60,15
> diff --git a/gas/testsuite/gas/ppc/power9.s b/gas/testsuite/gas/ppc/power9.s
> index 6ee49d4..9cf306c 100644
> --- a/gas/testsuite/gas/ppc/power9.s
> +++ b/gas/testsuite/gas/ppc/power9.s
> @@ -131,7 +131,9 @@ power9:
>  	xxextractuw 4,5,0x0
>  	xxextractuw 40,50,0xf
>  	xxspltib    4,0x0
> +	xxspltib    4,-256

Why would (and should) this now get accepted? It's out of range
even for signed 8-bit (and as we can see disassembles to 0).

Jan

>  	xxspltib    41,255
> +	xxspltib    41,-1
>  	xxinsertw   5,6,0
>  	xxinsertw   50,60,0xf
>  	xxbrh       6,7

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Alan Modra]

On Tue, May 17, 2016 at 01:58:51AM -0600, Jan Beulich wrote:
> >>> On 13.05.16 at 22:57, <bergner@vnet.ibm.com> wrote:
> > --- a/gas/testsuite/gas/ppc/power9.s
> > +++ b/gas/testsuite/gas/ppc/power9.s
> > @@ -131,7 +131,9 @@ power9:
> >  	xxextractuw 4,5,0x0
> >  	xxextractuw 40,50,0xf
> >  	xxspltib    4,0x0
> > +	xxspltib    4,-256
> 
> Why would (and should) this now get accepted? It's out of range
> even for signed 8-bit (and as we can see disassembles to 0).

I think it probably shouldn't be allowed.  PPC_OPERAND_SIGNOPT was
changed to accept a wider range in
https://sourceware.org/ml/binutils/2007-04/msg00252.html 
I didn't comment on why I made that change nor can I remember a good
reason, so it likely was accidental.

Peter, do you agree with the following?

	* config/tc-ppc.c (ppc_insert_operand): Trim PPC_OPERAND_SIGNOPT
	allowed negative range.
	* testsuite/gas/ppc/power9.s: Test xxspltib of -128, not -256.
	* testsuite/gas/ppc/power9.d: Update.

diff --git a/gas/config/tc-ppc.c b/gas/config/tc-ppc.c
index c3909c4..01b5c5a 100644
--- a/gas/config/tc-ppc.c
+++ b/gas/config/tc-ppc.c
@@ -1787,17 +1787,15 @@ ppc_insert_operand (unsigned long insn,
 
   if ((operand->flags & PPC_OPERAND_SIGNOPT) != 0)
     {
-      /* Extend the allowed range for addis to [-65536, 65535].
-	 Similarly for some VLE high part insns.  For 64-bit it
-	 would be good to disable this for signed fields since the
+      /* Extend the allowed range for addis to [-32768, 65535].
+	 Similarly for cmpli and some VLE high part insns.  For 64-bit
+	 it would be good to disable this for signed fields since the
 	 value is sign extended into the high 32 bits of the register.
 	 If the value is, say, an address, then we might care about
 	 the high bits.  However, gcc as of 2014-06 uses unsigned
 	 values when loading the high part of 64-bit constants using
-	 lis.
-	 Use the same extended range for cmpli, to allow at least
-	 [-32768, 65535].  */
-      min = ~max & -right;
+	 lis.  */
+      min = ~(max >> 1) & -right;
     }
   else if ((operand->flags & PPC_OPERAND_SIGNED) != 0)
     {
diff --git a/gas/testsuite/gas/ppc/power9.d b/gas/testsuite/gas/ppc/power9.d
index d84e635..a1d4681 100644
--- a/gas/testsuite/gas/ppc/power9.d
+++ b/gas/testsuite/gas/ppc/power9.d
@@ -140,7 +140,7 @@ Disassembly of section \.text:
 .*:	(f0 80 2a 94|94 2a 80 f0) 	xxextractuw vs4,vs5,0
 .*:	(f1 0f 92 97|97 92 0f f1) 	xxextractuw vs40,vs50,15
 .*:	(f0 80 02 d0|d0 02 80 f0) 	xxspltib vs4,0
-.*:	(f0 80 02 d0|d0 02 80 f0) 	xxspltib vs4,0
+.*:	(f0 84 02 d0|d0 02 84 f0) 	xxspltib vs4,128
 .*:	(f1 27 fa d1|d1 fa 27 f1) 	xxspltib vs41,255
 .*:	(f1 27 fa d1|d1 fa 27 f1) 	xxspltib vs41,255
 .*:	(f0 a0 32 d4|d4 32 a0 f0) 	xxinsertw vs5,vs6,0
diff --git a/gas/testsuite/gas/ppc/power9.s b/gas/testsuite/gas/ppc/power9.s
index 9cf306c..34576b7 100644
--- a/gas/testsuite/gas/ppc/power9.s
+++ b/gas/testsuite/gas/ppc/power9.s
@@ -131,7 +131,7 @@ power9:
 	xxextractuw 4,5,0x0
 	xxextractuw 40,50,0xf
 	xxspltib    4,0x0
-	xxspltib    4,-256
+	xxspltib    4,-128
 	xxspltib    41,255
 	xxspltib    41,-1
 	xxinsertw   5,6,0

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Peter Bergner]

On Wed, 2016-05-18 at 12:07 +0930, Alan Modra wrote:

> Peter, do you agree with the following?
> 
> 	> * config/tc-ppc.c (ppc_insert_operand): Trim PPC_OPERAND_SIGNOPT
> 	> allowed negative range.
> 	> * testsuite/gas/ppc/power9.s: Test xxspltib of -128, not -256.
> 	> * testsuite/gas/ppc/power9.d: Update.
> 

Hi Alan, yeah, that looks good to me.  Thanks for fixing this.

Jan, I didn't really mention this in my submission, but the main
rational for this change was to allow the use of -1 as a mask of
all 1 bits for fields like these.  One of our other GCC developers
thought this would simplify things in the GCC ppc port by being
able to use -1 for the mask value, rather than some unsigned mask
value that depends on the mask size.

Peter

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Jan Beulich]

>>> On 18.05.16 at 04:37, <amodra@gmail.com> wrote:
> --- a/gas/config/tc-ppc.c
> +++ b/gas/config/tc-ppc.c
> @@ -1787,17 +1787,15 @@ ppc_insert_operand (unsigned long insn,
>  
>    if ((operand->flags & PPC_OPERAND_SIGNOPT) != 0)
>      {
> -      /* Extend the allowed range for addis to [-65536, 65535].
> -	 Similarly for some VLE high part insns.  For 64-bit it
> -	 would be good to disable this for signed fields since the
> +      /* Extend the allowed range for addis to [-32768, 65535].
> +	 Similarly for cmpli and some VLE high part insns.  For 64-bit
> +	 it would be good to disable this for signed fields since the
>  	 value is sign extended into the high 32 bits of the register.
>  	 If the value is, say, an address, then we might care about
>  	 the high bits.  However, gcc as of 2014-06 uses unsigned
>  	 values when loading the high part of 64-bit constants using
> -	 lis.
> -	 Use the same extended range for cmpli, to allow at least
> -	 [-32768, 65535].  */
> -      min = ~max & -right;
> +	 lis.  */
> +      min = ~(max >> 1) & -right;
>      }

That's a much appreciated change: With the not so long ago added
addpcis/subpcis I had noticed a similar issue as with the xxspltib now
(and meant to kick off a thread asking for why it's that way there),
and the above change would apparently take care of both then.
The only odd thing left then is that subis is not symmetrical to addis,
while subpcis is wrt addpcis.

Thanks, Jan

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Alan Modra]

On Wed, May 18, 2016 at 03:37:13AM -0600, Jan Beulich wrote:
> The only odd thing left then is that subis is not symmetrical to addis,

Easily enough done.  There are no doubt more to fix yet:  I noticed that
the VLE e_add2i. is using the same operand type as e_add2is, which
can't be completely correct.

	* ppc-opc.c: Formatting.
	(NSISIGNOPT): Define.
	(powerpc_opcodes <subis>): Use NSISIGNOPT.

diff --git a/opcodes/ppc-opc.c b/opcodes/ppc-opc.c
index 04f2231..1c03c88 100644
--- a/opcodes/ppc-opc.c
+++ b/opcodes/ppc-opc.c
@@ -186,25 +186,25 @@ const struct powerpc_operand powerpc_operands[] =
      This sets the y bit of the BO field appropriately.  */
 #define BDM BDA + 1
   { 0xfffc, 0, insert_bdm, extract_bdm,
-      PPC_OPERAND_RELATIVE | PPC_OPERAND_SIGNED },
+    PPC_OPERAND_RELATIVE | PPC_OPERAND_SIGNED },
 
   /* The BD field in a B form instruction when the - modifier is used
      and absolute address is used.  */
 #define BDMA BDM + 1
   { 0xfffc, 0, insert_bdm, extract_bdm,
-      PPC_OPERAND_ABSOLUTE | PPC_OPERAND_SIGNED },
+    PPC_OPERAND_ABSOLUTE | PPC_OPERAND_SIGNED },
 
   /* The BD field in a B form instruction when the + modifier is used.
      This sets the y bit of the BO field appropriately.  */
 #define BDP BDMA + 1
   { 0xfffc, 0, insert_bdp, extract_bdp,
-      PPC_OPERAND_RELATIVE | PPC_OPERAND_SIGNED },
+    PPC_OPERAND_RELATIVE | PPC_OPERAND_SIGNED },
 
   /* The BD field in a B form instruction when the + modifier is used
      and absolute addressing is used.  */
 #define BDPA BDP + 1
   { 0xfffc, 0, insert_bdp, extract_bdp,
-      PPC_OPERAND_ABSOLUTE | PPC_OPERAND_SIGNED },
+    PPC_OPERAND_ABSOLUTE | PPC_OPERAND_SIGNED },
 
   /* The BF field in an X or XL form instruction.  */
 #define BF BDPA + 1
@@ -414,7 +414,8 @@ const struct powerpc_operand powerpc_operands[] =
 
   /* Power4 version for mfcr.  */
 #define FXM4 FXM + 1
-  { 0xff, 12, insert_fxm, extract_fxm, PPC_OPERAND_OPTIONAL | PPC_OPERAND_OPTIONAL_VALUE},
+  { 0xff, 12, insert_fxm, extract_fxm,
+    PPC_OPERAND_OPTIONAL | PPC_OPERAND_OPTIONAL_VALUE},
   /* If the FXM4 operand is ommitted, use the sentinel value -1.  */
   { -1, -1, NULL, NULL, 0},
 
@@ -493,10 +494,16 @@ const struct powerpc_operand powerpc_operands[] =
      SI field, only negated.  */
 #define NSI NBI + 1
   { 0xffff, 0, insert_nsi, extract_nsi,
-      PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED },
+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED },
+
+  /* The NSI field in a D form instruction when we accept a wide range
+     of positive values.  */
+#define NSISIGNOPT NSI + 1
+  { 0xffff, 0, NULL, NULL,
+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
 
   /* The RA field in an D, DS, DQ, X, XO, M, or MDS form instruction.  */
-#define RA NSI + 1
+#define RA NSISIGNOPT + 1
 #define RA_MASK (0x1f << 16)
   { 0x1f, 16, NULL, NULL, PPC_OPERAND_GPR },
 
@@ -601,7 +608,7 @@ const struct powerpc_operand powerpc_operands[] =
      SCLSCI8 field, only negated.  */
 #define SCLSCI8N SCLSCI8 + 1
   { 0xffffffff, PPC_OPSHIFT_INV, insert_sci8n, extract_sci8n,
-      PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED },
+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED },
 
   /* The SD field of the SD4 form instruction.  */
 #define SE_SD SCLSCI8N + 1
@@ -690,7 +697,8 @@ const struct powerpc_operand powerpc_operands[] =
   /* The TBR field in an XFX form instruction.  This is like the SPR
      field, but it is optional.  */
 #define TBR SV + 1
-  { 0x3ff, 11, insert_tbr, extract_tbr, PPC_OPERAND_OPTIONAL | PPC_OPERAND_OPTIONAL_VALUE},
+  { 0x3ff, 11, insert_tbr, extract_tbr,
+    PPC_OPERAND_OPTIONAL | PPC_OPERAND_OPTIONAL_VALUE},
   /* If the TBR operand is ommitted, use the value 268.  */
   { -1, 268, NULL, NULL, 0},
 
@@ -874,12 +882,12 @@ const struct powerpc_operand powerpc_operands[] =
   /* The VLESIMM field in a D form instruction.  */
 #define VLESIMM URC + 1
   { 0xffff, PPC_OPSHIFT_INV, insert_vlesi, extract_vlesi,
-      PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
+    PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
 
   /* The VLENSIMM field in a D form instruction.  */
 #define VLENSIMM VLESIMM + 1
   { 0xffff, PPC_OPSHIFT_INV, insert_vlensi, extract_vlensi,
-      PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
 
   /* The VLEUIMM field in a D form instruction.  */
 #define VLEUIMM VLENSIMM + 1
@@ -3878,7 +3886,7 @@ const struct powerpc_opcode powerpc_opcodes[] = {
 {"liu",		OP(15),		DRA_MASK,    PWRCOM,	PPCNONE,	{RT, SISIGNOPT}},
 {"addis",	OP(15),		OP_MASK,     PPCCOM,	PPCNONE,	{RT, RA0, SISIGNOPT}},
 {"cau",		OP(15),		OP_MASK,     PWRCOM,	PPCNONE,	{RT, RA0, SISIGNOPT}},
-{"subis",	OP(15),		OP_MASK,     PPCCOM,	PPCNONE,	{RT, RA0, NSI}},
+{"subis",	OP(15),		OP_MASK,     PPCCOM,	PPCNONE,	{RT, RA0, NSISIGNOPT}},
 
 {"bdnz-",    BBO(16,BODNZ,0,0),		BBOATBI_MASK,  PPCCOM,	 PPCNONE,	{BDM}},
 {"bdnz+",    BBO(16,BODNZ,0,0),		BBOATBI_MASK,  PPCCOM,	 PPCNONE,	{BDP}},

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Jan Beulich]

>>> Alan Modra <amodra@gmail.com> 05/19/16 6:50 AM >>>
>@@ -493,10 +494,16 @@ const struct powerpc_operand powerpc_operands[] =
>SI field, only negated.  */
>#define NSI NBI + 1
>{ 0xffff, 0, insert_nsi, extract_nsi,
>-      PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED },
>+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED },
>+
>+  /* The NSI field in a D form instruction when we accept a wide range
>+     of positive values.  */
>+#define NSISIGNOPT NSI + 1
>+  { 0xffff, 0, NULL, NULL,
>+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
 
Does this work without using an insertion function negating the intended value
(as it looks re-using {insert,extract}_nsi() should be okay)?

Jan

Re: [PATCH, ppc] Allow IMM8 operands to accept both signed and unsigned values

[Alan Modra]

On Wed, May 18, 2016 at 11:58:23PM -0600, Jan Beulich wrote:
> >>> Alan Modra <amodra@gmail.com> 05/19/16 6:50 AM >>>
> >+  /* The NSI field in a D form instruction when we accept a wide range
> >+     of positive values.  */
> >+#define NSISIGNOPT NSI + 1
> >+  { 0xffff, 0, NULL, NULL,
> >+    PPC_OPERAND_NEGATIVE | PPC_OPERAND_SIGNED | PPC_OPERAND_SIGNOPT },
>  
> Does this work without using an insertion function negating the intended value
> (as it looks re-using {insert,extract}_nsi() should be okay)?

Oops, indeed.  Fixed with the obvious change to use insert/extract.

-- 
Alan Modra
Australia Development Lab, IBM