Re: [PATCH v3,rs6000] Add built-in function support for Power9 byte instructions

[Sandra Loosemore]

On 12/12/2016 05:40 PM, Kelvin Nilsen wrote:


@@ -15105,6 +15109,24 @@ If all of the enabled test conditions are false, t
  The @code{scalar_test_neg} built-in functions return a non-zero value
  if their @code{source} argument holds a negative value.

+The @code{__builtin_byte_in_set} function requires a
+64-bit environment supporting ISA 3.0 or later.  This function returns
+a non-zero value if and only if its @code{u} argument exactly equals one of
+the eight bytes contained within its 64-bit @code{set} argument.
+
+The @code{__builtin_byte_in_range} and
+@code{__builtin_byte_in_either_range} require an environment
+supporting ISA 3.0 or later.  For these two functions, the
+@code{range} argument is encoded as 4 bytes, organized as
+@code{hi_1:lo_1:hi_2:lo_2}.
+The first of these functions returns a
+non-zero value if and only if its @code{u} argument is within the
+range bounded between @code{lo_2} and @code{hi_2} inclusive.
+The second of these functions returns non-zero if and only
+if its @code{u} argument is either within the range bounded between
+@code{lo_1} and @code{hi_1} inclusive or is within the range bounded between
+@code{lo_2} and @code{hi_2} inclusive.


I would prefer that you refer to the functions by name instead of "the 
first/second of these functions".


Also, you have a parallel construction problem in the second sentence: 
"is either within... or is within...".  I suggest rephrasing as "is 
within either...  or...".


-Sandra the nit-picky

Re: [PATCH v3,rs6000] Add built-in function support for Power9 byte instructions

[Segher Boessenkool]

On Tue, Dec 13, 2016 at 05:25:52PM -0700, Kelvin Nilsen wrote:
> Regarding the two test cases that are missing the scan-assembler
> directive (byte-in-set-1.c and byte-in-set-2.c), those tests are both
> expected to fail.  They are checking that the compiler rejects those
> programs with appropriate error messages.

Oh, apparently I cannot read.  Sorry about that.


Segher

Re: [PATCH v3,rs6000] Add built-in function support for Power9 byte instructions

[Kelvin Nilsen]

Thanks for your quick feedback.

I'll update the comments regarding possible future enhancement to
support QImode for operands[1] as well.

Regarding the two test cases that are missing the scan-assembler
directive (byte-in-set-1.c and byte-in-set-2.c), those tests are both
expected to fail.  They are checking that the compiler rejects those
programs with appropriate error messages.

On 12/13/2016 03:14 PM, Segher Boessenkool wrote:
> Hi Kelvin,
> 
> On Mon, Dec 12, 2016 at 05:40:05PM -0700, Kelvin Nilsen wrote:
>> The patch has been bootstrapped and tested on
>> powerpc64le-unknown-linux and powerpc-unknown-linux (big-endian, with
>> both -m32 and -m64 target options) with no regressions.
>>
>> Is this ok for the trunk?
> 
> Yes it is, much better, thanks!  Two comments below, please fix the testcase
> one before commit if it is indeed a problem:
> 
>> +;; Though the instructions to which this expansion maps operate on
>> +;; 64-bit registers, the current implementation only operates on
>> +;; SI-mode operands as the high-order bits provide no information
>> +;; that is not already available in the low-order bits.  To avoid the
>> +;; costs of data widening operations, a future enhancement might add
>> +;; support for DI-mode operands.
> 
> And operands[1] could be QImode.
> 
>> +(define_expand "cmprb"
>> +  [(set (match_dup 3)
>> +(unspec:CC [(match_operand:SI 1 "gpc_reg_operand" "r")
>> +(match_operand:SI 2 "gpc_reg_operand" "r")]
>> + UNSPEC_CMPRB))
> 
> 
>> --- gcc/testsuite/gcc.target/powerpc/byte-in-set-1.c (revision 0)
>> +++ gcc/testsuite/gcc.target/powerpc/byte-in-set-1.c (working copy)
> 
> Did you forget the scan-assembler here and in the next one, or do you only
> want to test it does indeed compile?
> 
> 
> Segher
> 
> 

-- 
Kelvin Nilsen, Ph.D.  kdnil...@linux.vnet.ibm.com
home office: 801-756-4821, cell: 520-991-6727
IBM Linux Technology Center - PPC Toolchain

Re: [PATCH v3,rs6000] Add built-in function support for Power9 byte instructions

[Segher Boessenkool]

Hi Kelvin,

On Mon, Dec 12, 2016 at 05:40:05PM -0700, Kelvin Nilsen wrote:
> The patch has been bootstrapped and tested on
> powerpc64le-unknown-linux and powerpc-unknown-linux (big-endian, with
> both -m32 and -m64 target options) with no regressions.
> 
> Is this ok for the trunk?

Yes it is, much better, thanks!  Two comments below, please fix the testcase
one before commit if it is indeed a problem:

> +;; Though the instructions to which this expansion maps operate on
> +;; 64-bit registers, the current implementation only operates on
> +;; SI-mode operands as the high-order bits provide no information
> +;; that is not already available in the low-order bits.  To avoid the
> +;; costs of data widening operations, a future enhancement might add
> +;; support for DI-mode operands.

And operands[1] could be QImode.

> +(define_expand "cmprb"
> +  [(set (match_dup 3)
> + (unspec:CC [(match_operand:SI 1 "gpc_reg_operand" "r")
> + (match_operand:SI 2 "gpc_reg_operand" "r")]
> +  UNSPEC_CMPRB))


> --- gcc/testsuite/gcc.target/powerpc/byte-in-set-1.c  (revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/byte-in-set-1.c  (working copy)

Did you forget the scan-assembler here and in the next one, or do you only
want to test it does indeed compile?


Segher

[PATCH v3,rs6000] Add built-in function support for Power9 byte instructions

[Kelvin Nilsen]

This patch adds built-in function support for the new setb, cmprb, and
cmpeqb Power9 instructions.  This third version of the patch differs
from the second in the following ways:

1. Changed the name of the *cmprb, *setb, *cmprb2, and *cmpeqb new
instructions to *cmprb_internal, *setb_internal, and *cmprb2_internal
respectively.

2. Added comments to the cmprb, setb, and cmprb2 instructions to
acknowledge that, as implemented, we do not currently support the use
of double-integer operands though support for this might be added in
the future.

3. Changed the names of the new non-overloaded builtin functions to 
be of the form __builtin_scalar_ instead of
__builtin_altivec_.  Changed the names of the new overloaded
functions to be of the form __builtin_ instead of
__builtin_scalar_.

4. Corrected the comments describing range encodings and simplified the
descriptions by speaking of individual bytes instead of bit numbers
(cmprb, cmprb2, cmpeqb define_expand patterns and *cmprb_internal,
*cmprb2_internal, *cmpeqb_internal define_insn patterns).

5. Updated documentation to use the new function names and to speak of
range encodings in terms of individual bytes instead of bit numbers.

6. Changed the test cases to use the new function names.

7. Corrected bit shifting of arguments in the byte-in-range-0.c and
byte-in-range-1.c test cases.


The patch has been bootstrapped and tested on
powerpc64le-unknown-linux and powerpc-unknown-linux (big-endian, with
both -m32 and -m64 target options) with no regressions.

Is this ok for the trunk?

gcc/testsuite/ChangeLog:

2016-12-12  Kelvin Nilsen  

* gcc.target/powerpc/byte-in-either-range-0.c: New test.
* gcc.target/powerpc/byte-in-either-range-1.c: New test.
* gcc.target/powerpc/byte-in-range-0.c: New test.
* gcc.target/powerpc/byte-in-range-1.c: New test.
* gcc.target/powerpc/byte-in-set-0.c: New test.
* gcc.target/powerpc/byte-in-set-1.c: New test.
* gcc.target/powerpc/byte-in-set-2.c: New test.


gcc/ChangeLog:

2016-12-12  Kelvin Nilsen  

* config/rs6000/altivec.md (UNSPEC_CMPRB): New unspec value.
(UNSPEC_CMPRB2): New unspec value.
(UNSPEC_CMPEQB): New unspec value.
(cmprb): New expansion.
(*cmprb_internal): New insn.
(*setb_internal): New insn.
(cmprb2): New expansion.
(*cmprb2_internal): New insn.
(cmpeqb): New expansion.
(*cmpeqb_internal): New insn.
* config/rs6000/rs6000-builtin.def (BU_P9_2): New macro.
(BU_P9_64BIT_2): Likewise.
(BU_P9_OVERLOAD_2): Likewise.
(CMPRB): Add byte-in-range built-in function.
(CMBRB2): Add byte-in-either-range built-in function.
(CMPEQB): Add byte-in-set built-in function.
(CMPRB): Add overload support for byte-in-range function.
(CMPRB2): Add overload support for byte-in-either-range function.
(CMPEQB): Add overload support for byte-in-set built-in function.
* config/rs6000/rs6000-c.c (P9_BUILTIN_CMPRB): Macro expansion to
define argument types for new builtin. 
(P9_BUILTIN_CMPRB2): Likewise.
(P9_BUILTIN_CMPEQB): Likewise.
* doc/extend.texi (PowerPC AltiVec Built-in Functions): Rearrange
the order of presentation for certain built-in functions
(scalar_extract_exp, scalar_extract_sig, scalar_insert_exp)
(scalar_cmp_exp_gt, scalar_cmp_exp_lt, scalar_cmp_exp_eq)
(scalar_cmp_exp_unordered, scalar_test_data_class)
(scalar_test_neg) to improve locality and flow.  Document
the new __builtin_scalar_byte_in_set,
__builtin_scalar_byte_in_range, and
__builtin_scalar_byte_in_either_range functions.

Index: gcc/config/rs6000/altivec.md
===
--- gcc/config/rs6000/altivec.md(revision 241245)
+++ gcc/config/rs6000/altivec.md(working copy)
@@ -153,6 +153,9 @@
UNSPEC_BCDADD
UNSPEC_BCDSUB
UNSPEC_BCD_OVERFLOW
+   UNSPEC_CMPRB
+   UNSPEC_CMPRB2
+   UNSPEC_CMPEQB
 ])
 
 (define_c_enum "unspecv"
@@ -3709,6 +3712,189 @@
   "darn %0,1"
   [(set_attr "type" "integer")])
 
+;; Test byte within range.
+;;
+;; The bytes of operand 1 are organized as xx:xx:xx:vv, where xx
+;; represents a byte whose value is ignored in this context and
+;; vv, the least significant byte, holds the byte value that is to
+;; be tested for membership within the range specified by operand 2.
+;; The bytes of operand 2 are organized as xx:xx:hi:lo.
+;;
+;; Return in target register operand 0 a value of 1 if lo <= vv and
+;; vv <= hi.  Otherwise, set register operand 0 to 0.
+;;
+;; Though the instructions to which this expansion maps operate on
+;; 64-bit registers, the current implementation only operates on
+;; SI-mode operands as the high-order bits provide no information
+;; that is not already available in the low-order bits.  To