Message ID | 20110330180925.GA1243@intel.com |
---|---|
State | New |
Headers | show |
On Wed, Mar 30, 2011 at 8:09 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: > On Wed, Mar 30, 2011 at 08:02:38AM -0700, H.J. Lu wrote: >> Hi, >> >> Currently, we limit XVECEXP to 26 elements in machine description >> since we use letters 'a' to 'z' to encode them. I don't see any >> reason why we can't go beyond 'z'. This patch removes this restriction. >> Any comments? >> > > That was wrong. The problem is in vector elements. This patch passes > bootstrap. Any comments? Do you really need it? Richard. > Thanks. > > > H.J. > --- > 2011-03-30 H.J. Lu <hongjiu.lu@intel.com> > > * genrecog.c (VECTOR_ELEMENT_BASE): New. > (add_to_sequence): Add assert. Use VECTOR_ELEMENT_BASE to > encode vector elements. > (change_state): Check and support VECTOR_ELEMENT_BASE. > (make_insn_sequence): Add assert. > > diff --git a/gcc/genrecog.c b/gcc/genrecog.c > index 74dd0a7..40e9c4d 100644 > --- a/gcc/genrecog.c > +++ b/gcc/genrecog.c > @@ -465,6 +465,9 @@ extern void debug_decision > (struct decision *); > extern void debug_decision_list > (struct decision *); > + > +/* The base of vector element. */ > +#define VECTOR_ELEMENT_BASE 0x80 > > /* Create a new node in sequence after LAST. */ > > @@ -912,6 +915,7 @@ add_to_sequence (rtx pattern, struct decision_head *last, const char *position, > /* Which insn we're looking at is represented by A-Z. We don't > ever use 'A', however; it is always implied. */ > > + gcc_assert (i < 26); > subpos[depth] = (i > 0 ? 'A' + i : 0); > sub = add_to_sequence (XVECEXP (pattern, 0, i), > last, subpos, insn_type, 0); > @@ -1002,6 +1006,9 @@ add_to_sequence (rtx pattern, struct decision_head *last, const char *position, > char base = (was_code == MATCH_OPERATOR ? '0' : 'a'); > for (i = 0; i < (size_t) XVECLEN (pattern, 2); i++) > { > + gcc_assert (was_code == MATCH_OPERATOR > + ? ISDIGIT (i + base) > + : ISLOWER (i + base)); > subpos[depth] = i + base; > sub = add_to_sequence (XVECEXP (pattern, 2, i), > &sub->success, subpos, insn_type, 0); > @@ -1102,7 +1109,9 @@ add_to_sequence (rtx pattern, struct decision_head *last, const char *position, > int j; > for (j = 0; j < XVECLEN (pattern, i); j++) > { > - subpos[depth] = 'a' + j; > + int val = j + VECTOR_ELEMENT_BASE; > + gcc_assert (val <= UCHAR_MAX); > + subpos[depth] = val; > sub = add_to_sequence (XVECEXP (pattern, i, j), > &sub->success, subpos, insn_type, 0); > } > @@ -1779,6 +1788,10 @@ change_state (const char *oldpos, const char *newpos, const char *indent) > else if (ISLOWER (newpos[depth])) > printf ("%sx%d = XVECEXP (x%d, 0, %d);\n", > indent, depth + 1, depth, newpos[depth] - 'a'); > + else if (((unsigned char) newpos[depth]) >= VECTOR_ELEMENT_BASE) > + printf ("%sx%d = XVECEXP (x%d, 0, %d);\n", > + indent, depth + 1, depth, > + ((unsigned char) newpos[depth]) - VECTOR_ELEMENT_BASE); > else > printf ("%sx%d = XEXP (x%d, %c);\n", > indent, depth + 1, depth, newpos[depth]); > @@ -2528,6 +2541,7 @@ make_insn_sequence (rtx insn, enum routine_type type) > } > XVECLEN (x, 0) = j; > > + gcc_assert ((j - 1) < 26); > c_test_pos[0] = 'A' + j - 1; > c_test_pos[1] = '\0'; > } >
Richard Guenther <richard.guenther@gmail.com> writes: > On Wed, Mar 30, 2011 at 8:09 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> On Wed, Mar 30, 2011 at 08:02:38AM -0700, H.J. Lu wrote: >>> Hi, >>> >>> Currently, we limit XVECEXP to 26 elements in machine description >>> since we use letters 'a' to 'z' to encode them. I don't see any >>> reason why we can't go beyond 'z'. This patch removes this restriction. >>> Any comments? >>> >> >> That was wrong. The problem is in vector elements. This patch passes >> bootstrap. Any comments? > > Do you really need it? ISTR at least two separate projects have hit the limit. This solution seems very hackish though (as well as being ASCII-specific). A better fix might be to replace "char" with a proper type, such as a union and discriminator. Richard
On Mar 31, 2011, at 1:41 AM, Richard Guenther wrote: > On Wed, Mar 30, 2011 at 8:09 PM, H.J. Lu <hongjiu.lu@intel.com> wrote: >> On Wed, Mar 30, 2011 at 08:02:38AM -0700, H.J. Lu wrote: >>> Hi, >>> >>> Currently, we limit XVECEXP to 26 elements in machine description >>> since we use letters 'a' to 'z' to encode them. I don't see any >>> reason why we can't go beyond 'z'. This patch removes this restriction. >>> Any comments? >>> >> >> That was wrong. The problem is in vector elements. This patch passes >> bootstrap. Any comments? > > Do you really need it? I'm trying to recall if this is the limit Kenny and I hit.... If so, annoying. Kenny could confirm if it was. gcc's general strategy of, no fixed N gives gcc a certain flexibility that is very nice to have, on those general grounds, I kinda liked this patch.
we hit this limit trying to write the explicit semantics for a vec_interleave_evenv32qi. ;;(define_insn "vec_interleave_evenv32qi" ;; [(set (match_operand:V32QI 0 "register_operand" "=r") ;; (vec_select:V32QI ;; (vec_concat:V64QI ;; (match_operand:V32QI 1 "register_operand" "0") ;; (match_operand:V32QI 2 "register_operand" "r")) ;; (parallel [(const_int 0) (const_int 32) ;; (const_int 2) (const_int 34) ;; (const_int 4) (const_int 36) ;; (const_int 6) (const_int 38) ;; (const_int 8) (const_int 40) ;; (const_int 10) (const_int 42) ;; (const_int 12) (const_int 44) ;; (const_int 14) (const_int 46) ;; (const_int 16) (const_int 48) ;; (const_int 18) (const_int 50) ;; (const_int 20) (const_int 52) ;; (const_int 22) (const_int 54) ;; (const_int 24) (const_int 56) ;; (const_int 26) (const_int 58) ;; (const_int 28) (const_int 60) ;; (const_int 30) (const_int 62)])))] ;; "" ;; "rimihv\t%0,%2,8,15,8" ;; [(set_attr "type" "rimi")]) kenny On 03/31/2011 06:16 AM, Mike Stump wrote: > On Mar 31, 2011, at 1:41 AM, Richard Guenther wrote: >> On Wed, Mar 30, 2011 at 8:09 PM, H.J. Lu<hongjiu.lu@intel.com> wrote: >>> On Wed, Mar 30, 2011 at 08:02:38AM -0700, H.J. Lu wrote: >>>> Hi, >>>> >>>> Currently, we limit XVECEXP to 26 elements in machine description >>>> since we use letters 'a' to 'z' to encode them. I don't see any >>>> reason why we can't go beyond 'z'. This patch removes this restriction. >>>> Any comments? >>>> >>> That was wrong. The problem is in vector elements. This patch passes >>> bootstrap. Any comments? >> Do you really need it? > I'm trying to recall if this is the limit Kenny and I hit.... If so, annoying. Kenny could confirm if it was. gcc's general strategy of, no fixed N gives gcc a certain flexibility that is very nice to have, on those general grounds, I kinda liked this patch.
On Thu, Mar 31, 2011 at 5:05 AM, Kenneth Zadeck <zadeck@naturalbridge.com> wrote: > we hit this limit trying to write the explicit semantics for a > vec_interleave_evenv32qi. > > ;;(define_insn "vec_interleave_evenv32qi" > ;; [(set (match_operand:V32QI 0 "register_operand" "=r") > ;; (vec_select:V32QI > ;; (vec_concat:V64QI > ;; (match_operand:V32QI 1 "register_operand" "0") > ;; (match_operand:V32QI 2 "register_operand" "r")) > ;; (parallel [(const_int 0) (const_int 32) > ;; (const_int 2) (const_int 34) > ;; (const_int 4) (const_int 36) > ;; (const_int 6) (const_int 38) > ;; (const_int 8) (const_int 40) > ;; (const_int 10) (const_int 42) > ;; (const_int 12) (const_int 44) > ;; (const_int 14) (const_int 46) > ;; (const_int 16) (const_int 48) > ;; (const_int 18) (const_int 50) > ;; (const_int 20) (const_int 52) > ;; (const_int 22) (const_int 54) > ;; (const_int 24) (const_int 56) > ;; (const_int 26) (const_int 58) > ;; (const_int 28) (const_int 60) > ;; (const_int 30) (const_int 62)])))] > ;; "" > ;; "rimihv\t%0,%2,8,15,8" > ;; [(set_attr "type" "rimi")]) > > > kenny > > On 03/31/2011 06:16 AM, Mike Stump wrote: >> >> On Mar 31, 2011, at 1:41 AM, Richard Guenther wrote: >>> >>> On Wed, Mar 30, 2011 at 8:09 PM, H.J. Lu<hongjiu.lu@intel.com> wrote: >>>> >>>> On Wed, Mar 30, 2011 at 08:02:38AM -0700, H.J. Lu wrote: >>>>> >>>>> Hi, >>>>> >>>>> Currently, we limit XVECEXP to 26 elements in machine description >>>>> since we use letters 'a' to 'z' to encode them. I don't see any >>>>> reason why we can't go beyond 'z'. This patch removes this >>>>> restriction. >>>>> Any comments? >>>>> >>>> That was wrong. The problem is in vector elements. This patch passes >>>> bootstrap. Any comments? >>> >>> Do you really need it? >> >> I'm trying to recall if this is the limit Kenny and I hit.... If so, >> annoying. Kenny could confirm if it was. gcc's general strategy of, no >> fixed N gives gcc a certain flexibility that is very nice to have, on those >> general grounds, I kinda liked this patch. > Is my patch OK to install? Thanks.
diff --git a/gcc/genrecog.c b/gcc/genrecog.c index 74dd0a7..40e9c4d 100644 --- a/gcc/genrecog.c +++ b/gcc/genrecog.c @@ -465,6 +465,9 @@ extern void debug_decision (struct decision *); extern void debug_decision_list (struct decision *); + +/* The base of vector element. */ +#define VECTOR_ELEMENT_BASE 0x80 /* Create a new node in sequence after LAST. */ @@ -912,6 +915,7 @@ add_to_sequence (rtx pattern, struct decision_head *last, const char *position, /* Which insn we're looking at is represented by A-Z. We don't ever use 'A', however; it is always implied. */ + gcc_assert (i < 26); subpos[depth] = (i > 0 ? 'A' + i : 0); sub = add_to_sequence (XVECEXP (pattern, 0, i), last, subpos, insn_type, 0); @@ -1002,6 +1006,9 @@ add_to_sequence (rtx pattern, struct decision_head *last, const char *position, char base = (was_code == MATCH_OPERATOR ? '0' : 'a'); for (i = 0; i < (size_t) XVECLEN (pattern, 2); i++) { + gcc_assert (was_code == MATCH_OPERATOR + ? ISDIGIT (i + base) + : ISLOWER (i + base)); subpos[depth] = i + base; sub = add_to_sequence (XVECEXP (pattern, 2, i), &sub->success, subpos, insn_type, 0); @@ -1102,7 +1109,9 @@ add_to_sequence (rtx pattern, struct decision_head *last, const char *position, int j; for (j = 0; j < XVECLEN (pattern, i); j++) { - subpos[depth] = 'a' + j; + int val = j + VECTOR_ELEMENT_BASE; + gcc_assert (val <= UCHAR_MAX); + subpos[depth] = val; sub = add_to_sequence (XVECEXP (pattern, i, j), &sub->success, subpos, insn_type, 0); } @@ -1779,6 +1788,10 @@ change_state (const char *oldpos, const char *newpos, const char *indent) else if (ISLOWER (newpos[depth])) printf ("%sx%d = XVECEXP (x%d, 0, %d);\n", indent, depth + 1, depth, newpos[depth] - 'a'); + else if (((unsigned char) newpos[depth]) >= VECTOR_ELEMENT_BASE) + printf ("%sx%d = XVECEXP (x%d, 0, %d);\n", + indent, depth + 1, depth, + ((unsigned char) newpos[depth]) - VECTOR_ELEMENT_BASE); else printf ("%sx%d = XEXP (x%d, %c);\n", indent, depth + 1, depth, newpos[depth]); @@ -2528,6 +2541,7 @@ make_insn_sequence (rtx insn, enum routine_type type) } XVECLEN (x, 0) = j; + gcc_assert ((j - 1) < 26); c_test_pos[0] = 'A' + j - 1; c_test_pos[1] = '\0'; }