Thank you very much for the prompt and thorough review. There are a few points below where I'd like to seek further clarification.
On 11/15/2016 04:19 AM, Segher Boessenkool wrote: > 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? I appreciate your preference to avoid proliferation of special-case unspec constants. However, it is a not so straightforward to combine these two cases under the same constant value. The issue is that though the two encoding conceptually represent different "numbers of arguments", the arguments are all packed inside of a 32-bit register. At the RTL level, it looks like the two different forms have the same number of arguments (the same number of register operands). The difference is which bits serve relevant purposes within the incoming register operands. So I'm inclined to keep this as is if that's ok with you. > >> (cmprb_p): New expansion. > > Not such a great name (now you get a gen_cmprb_p function which isn't > a predicate itself). I'll change these names. > >> (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? Thanks. > >> +;; 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. > Thanks for catching this. I think I got endian confusion inside my head while I was writing the above. I will rewrite these comments, below also. >> +;; 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? > The reason it seems most "natural" to me use the expanders is because I need to introduce a temporary CR scratch register between expansion and insn matching. Also, it seems that the *setb pattern may be of more general use in the future implementation of other built-in functions. I'm inclined to keep this as is, but if you still feel otherwise, I'll figure out how to avoid the expansion.
