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

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Mon, Nov 14, 2016 at 04:43:35PM -0700, Kelvin Nilsen wrote:
> 	* config/rs6000/altivec.md (UNSPEC_CMPRB): New unspec value.
> 	(UNSPEC_CMPRB2): New unspec value.

I wonder if you really need both?  The number of arguments will tell
which is which, anyway?

> 	(cmprb_p): New expansion.

Not such a great name (now you get a gen_cmprb_p function which isn't
a predicate itself).

> 	(CMPRB): Add byte-in-range built-in function.
> 	(CMBRB2): Add byte-in-either_range built-in function.
> 	(CMPEQB): Add byte-in-set builtin-in function.

"builtin-in", and you typoed an underscore?

> +;; Predicate: test byte within range.
> +;; Return in target register operand 0 a non-zero value iff the byte
> +;; held in bits 24:31 of operand 1 is within the inclusive range
> +;; bounded below by operand 2's bits 0:7 and above by operand 2's
> +;; bits 8:15.
> +(define_expand "cmprb_p"

It seems you got the bit numbers mixed up.  Maybe just call it the low
byte, and the byte just above?

(And it always sets 0 or 1 here, you might want to make that more explicit).

> +;; Set bit 1 (the GT bit, 0x2) of CR register operand 0 to 1 iff the

That's 4, i.e. 0b0100.

> +;; Set operand 0 register to non-zero value iff the CR register named
> +;; by operand 1 has its GT bit (0x2) or its LT bit (0x1) set.
> +(define_insn "*setb"

LT is 8, GT is 4.  If LT is set it returns -1, otherwise if GT is set it
returns 1, otherwise it returns 0.

> +;; Predicate: test byte within two ranges.
> +;; Return in target register operand 0 a non-zero value iff the byte
> +;; held in bits 24:31 of operand 1 is within the inclusive range
> +;; bounded below by operand 2's bits 0:7 and above by operand 2's
> +;; bits 8:15 or if the byte is within the inclusive range bounded
> +;; below by operand 2's bits 16:23 and above by operand 2's bits 24:31.
> +(define_expand "cmprb2_p"

The high bound is higher in the reg than the low bound.  See the example
where 0x3930 is used to do isdigit (and yes 0x3039 would be much more
fun, but alas).

> +;; Predicate: test byte membership within set of 8 bytes.
> +;; Return in target register operand 0 a non-zero value iff the byte
> +;; held in bits 24:31 of operand 1 equals at least one of the eight
> +;; byte values represented by the 64-bit register supplied as operand
> +;; 2.  Note that the 8 byte values held within operand 2 need not be
> +;; unique. 

(trailing space)

I wonder if we really need all these predicate expanders, if it wouldn't
be easier if the builtin handling code did the setb itself?


Segher