* RFA: cache recog_op_alt by insn code @ 2014-05-20 8:19 Richard Sandiford 2014-05-20 18:04 ` Jeff Law 0 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-20 8:19 UTC (permalink / raw) To: gcc-patches Following on from (and depending on) the last patch, process_constraints also shows up high in the profile. This patch caches the recog_op_alt information by insn code too. It also shrinks the size of the structure from 1 pointer + 5 ints to 1 pointer + 2 ints: - no target should have more than 65536 register classes :-) - "reject" is based on a cost of 600 for '!', so 16 bits should be plenty - "matched" and "matches" are operand numbers and so fit in 8 bits Again it seems LRA already did the same thing locally. It has a routine setup_operand_alternative that is mostly a direct cut-&-paste of process_constraints. AFAICT the only difference is that it sets up some parts of the main lra_static_insn_data structure too. Since this code is creating cached data and should only be run once per insn code or asm statement, I don't think the extra in-loop lra_static_insn_data assignments really justify having two copies of such a complicated function. I think it would be better for LRA to use the generic routine and then fill in the lra_static_insn_data fields on the result (basically just an OR of "is_address" and "early_clobber" for each alternative, plus setting up "commutative"). We could then avoid having two caches of the same data. I'll do that as a follow-up if it sounds OK. On the subject of commutativity, we have: static bool commutative_constraint_p (const char *str) { int curr_alt, c; bool ignore_p; for (ignore_p = false, curr_alt = 0;;) { c = *str; if (c == '\0') break; str += CONSTRAINT_LEN (c, str); if (c == '#' || !recog_data.alternative_enabled_p[curr_alt]) ignore_p = true; else if (c == ',') { curr_alt++; ignore_p = false; } else if (! ignore_p) { /* Usually `%' is the first constraint character but the documentation does not require this. */ if (c == '%') return true; } } return false; } Any objections to requiring `%' to be the first constraint character? Seems wasteful to be searching the constraint string just to support an odd case. The patch gives a further ~3.5% improvement in compile time for -O0 fold-const.ii, on top of the other patch. Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * recog.h (operand_alternative): Convert reg_class, reject, matched and matches into bitfields. (preprocess_constraints): Take the insn as parameter. (recog_op_alt): Change into an array of pointers. (target_recog): Add x_op_alt. * recog.c (asm_op_alt_1, asm_op_alt): New variables (recog_op_alt): Change into an array of pointers. (preprocess_constraints): Update accordingly. Take the insn as parameter. Use asm_op_alt_1 and asm_op_alt for asms. Cache other instructions in this_target_recog. * ira-lives.c (process_bb_node_lives): Pass the insn to process_constraints. * reg-stack.c (check_asm_stack_operands): Likewise. (subst_asm_stack_regs): Likewise. * regcprop.c (copyprop_hardreg_forward_1): Likewise. * regrename.c (build_def_use): Likewise. * sched-deps.c (sched_analyze_insn): Likewise. * sel-sched.c (get_reg_class, implicit_clobber_conflict_p): Likewise. * config/arm/arm.c (xscale_sched_adjust_cost): Likewise. (note_invalid_constants): Likewise. * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. Index: gcc/recog.h =================================================================== --- gcc/recog.h 2014-05-19 20:42:50.830279171 +0100 +++ gcc/recog.h 2014-05-19 21:02:22.926604180 +0100 @@ -46,18 +46,18 @@ struct operand_alternative const char *constraint; /* The register class valid for this alternative (possibly NO_REGS). */ - enum reg_class cl; + ENUM_BITFIELD (reg_class) cl : 16; /* "Badness" of this alternative, computed from number of '?' and '!' characters in the constraint string. */ - unsigned int reject; + unsigned int reject : 16; /* -1 if no matching constraint was found, or an operand number. */ - int matches; + int matches : 8; /* The same information, but reversed: -1 if this operand is not matched by any other, or the operand number of the operand that matches this one. */ - int matched; + int matched : 8; /* Nonzero if '&' was found in the constraint string. */ unsigned int earlyclobber:1; @@ -77,8 +77,9 @@ struct operand_alternative /* Nonzero if 'X' was found in the constraint string, or if the constraint string for this alternative was empty. */ unsigned int anything_ok:1; -}; + unsigned int unused : 8; +}; extern void init_recog (void); extern void init_recog_no_volatile (void); @@ -134,7 +135,7 @@ extern void insn_extract (rtx); extern void extract_insn (rtx); extern void extract_constrain_insn_cached (rtx); extern void extract_insn_cached (rtx); -extern void preprocess_constraints (void); +extern void preprocess_constraints (rtx); extern rtx peep2_next_insn (int); extern int peep2_regno_dead_p (int, int); extern int peep2_reg_dead_p (int, rtx); @@ -258,7 +259,7 @@ struct recog_data_d /* Contains a vector of operand_alternative structures for every operand. Set up by preprocess_constraints. */ -extern struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS][MAX_RECOG_ALTERNATIVES]; +extern operand_alternative **recog_op_alt; /* A table defined in insn-output.c that give information about each insn-code value. */ @@ -376,6 +377,7 @@ struct insn_data_d /* Target-dependent globals. */ struct target_recog { alternative_mask x_enabled_alternatives[LAST_INSN_CODE]; + operand_alternative **x_op_alt[LAST_INSN_CODE]; }; extern struct target_recog default_target_recog; Index: gcc/recog.c =================================================================== --- gcc/recog.c 2014-05-19 20:43:28.978647678 +0100 +++ gcc/recog.c 2014-05-19 23:26:02.037573014 +0100 @@ -80,7 +80,11 @@ struct recog_data_d recog_data; /* Contains a vector of operand_alternative structures for every operand. Set up by preprocess_constraints. */ -struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS][MAX_RECOG_ALTERNATIVES]; +operand_alternative **recog_op_alt; + +static operand_alternative asm_op_alt_1[MAX_RECOG_OPERANDS + * MAX_RECOG_ALTERNATIVES]; +static operand_alternative *asm_op_alt[MAX_RECOG_OPERANDS]; /* On return from `constrain_operands', indicate which alternative was satisfied. */ @@ -2326,23 +2330,43 @@ extract_insn (rtx insn) information from the constraint strings into a more usable form. The collected data is stored in recog_op_alt. */ void -preprocess_constraints (void) +preprocess_constraints (rtx insn) { int i; - for (i = 0; i < recog_data.n_operands; i++) - memset (recog_op_alt[i], 0, (recog_data.n_alternatives - * sizeof (struct operand_alternative))); + int code = INSN_CODE (insn); + if (code >= 0 && this_target_recog->x_op_alt[code]) + { + recog_op_alt = this_target_recog->x_op_alt[code]; + return; + } + + int n_alternatives = recog_data.n_alternatives; + int n_operands = recog_data.n_operands; + int n_entries = n_operands * n_alternatives; + + operand_alternative *op_alt; + if (code < 0) + { + memset (asm_op_alt_1, 0, n_entries * sizeof (operand_alternative)); + op_alt = asm_op_alt_1; + recog_op_alt = asm_op_alt; + } + else + { + op_alt = XCNEWVEC (operand_alternative, n_entries); + recog_op_alt = XNEWVEC (operand_alternative *, n_operands); + this_target_recog->x_op_alt[code] = recog_op_alt; + } - for (i = 0; i < recog_data.n_operands; i++) + for (i = 0; i < n_operands; i++, op_alt += n_alternatives) { int j; - struct operand_alternative *op_alt; const char *p = recog_data.constraints[i]; - op_alt = recog_op_alt[i]; + recog_op_alt[i] = op_alt; - for (j = 0; j < recog_data.n_alternatives; j++) + for (j = 0; j < n_alternatives; j++) { op_alt[j].cl = NO_REGS; op_alt[j].constraint = p; Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2014-05-19 20:42:50.819279065 +0100 +++ gcc/ira-lives.c 2014-05-19 21:01:12.577922691 +0100 @@ -1238,7 +1238,7 @@ process_bb_node_lives (ira_loop_tree_nod } extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); process_single_reg_class_operands (false, freq); /* See which defined values die here. */ Index: gcc/reg-stack.c =================================================================== --- gcc/reg-stack.c 2014-05-16 09:47:34.336936052 +0100 +++ gcc/reg-stack.c 2014-05-19 21:01:50.349288768 +0100 @@ -473,7 +473,7 @@ check_asm_stack_operands (rtx insn) constrain_operands (1); alt = which_alternative; - preprocess_constraints (); + preprocess_constraints (insn); get_asm_operands_in_out (body, &n_outputs, &n_inputs); @@ -2032,7 +2032,7 @@ subst_asm_stack_regs (rtx insn, stack_pt constrain_operands (1); alt = which_alternative; - preprocess_constraints (); + preprocess_constraints (insn); get_asm_operands_in_out (body, &n_outputs, &n_inputs); Index: gcc/regcprop.c =================================================================== --- gcc/regcprop.c 2014-05-17 07:59:06.436021428 +0100 +++ gcc/regcprop.c 2014-05-19 21:01:29.001081988 +0100 @@ -774,7 +774,7 @@ copyprop_hardreg_forward_1 (basic_block extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); alt = which_alternative; n_ops = recog_data.n_operands; is_asm = asm_noperands (PATTERN (insn)) >= 0; @@ -880,7 +880,7 @@ copyprop_hardreg_forward_1 (basic_block extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); } /* Otherwise, try all valid registers and see if its valid. */ @@ -908,7 +908,7 @@ copyprop_hardreg_forward_1 (basic_block extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); } } } Index: gcc/regrename.c =================================================================== --- gcc/regrename.c 2014-05-06 18:38:47.928200023 +0100 +++ gcc/regrename.c 2014-05-19 21:01:35.724147073 +0100 @@ -1571,7 +1571,7 @@ build_def_use (basic_block bb) extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); alt = which_alternative; n_ops = recog_data.n_operands; untracked_operands = 0; Index: gcc/sched-deps.c =================================================================== --- gcc/sched-deps.c 2014-05-16 09:47:34.347936161 +0100 +++ gcc/sched-deps.c 2014-05-19 21:01:58.526367964 +0100 @@ -2865,7 +2865,7 @@ sched_analyze_insn (struct deps_desc *de HARD_REG_SET temp; extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); ira_implicitly_set_insn_hard_regs (&temp); AND_COMPL_HARD_REG_SET (temp, ira_no_alloc_regs); IOR_HARD_REG_SET (implicit_reg_pending_clobbers, temp); Index: gcc/sel-sched.c =================================================================== --- gcc/sel-sched.c 2014-03-04 21:19:43.120097702 +0000 +++ gcc/sel-sched.c 2014-05-19 21:02:14.471522363 +0100 @@ -1019,7 +1019,7 @@ get_reg_class (rtx insn) extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); alt = which_alternative; n_ops = recog_data.n_operands; @@ -2141,7 +2141,7 @@ implicit_clobber_conflict_p (insn_t thro /* Calculate implicit clobbers. */ extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); ira_implicitly_set_insn_hard_regs (&temp); AND_COMPL_HARD_REG_SET (temp, ira_no_alloc_regs); Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c 2014-05-19 07:46:29.013179879 +0100 +++ gcc/config/arm/arm.c 2014-05-19 21:05:50.289605508 +0100 @@ -11335,7 +11335,7 @@ xscale_sched_adjust_cost (rtx insn, rtx that overlaps with SHIFTED_OPERAND, then we have increase the cost of this dependency. */ extract_insn (dep); - preprocess_constraints (); + preprocess_constraints (dep); for (opno = 0; opno < recog_data.n_operands; opno++) { /* We can ignore strict inputs. */ @@ -16870,7 +16870,7 @@ note_invalid_constants (rtx insn, HOST_W /* Fill in recog_op_alt with information about the constraints of this insn. */ - preprocess_constraints (); + preprocess_constraints (insn); for (opno = 0; opno < recog_data.n_operands; opno++) { Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c 2014-05-19 07:46:26.771160339 +0100 +++ gcc/config/i386/i386.c 2014-05-19 21:05:39.461501255 +0100 @@ -5826,7 +5826,7 @@ ix86_legitimate_combined_insn (rtx insn) int i; extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); for (i = 0; i < recog_data.n_operands; i++) { ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: RFA: cache recog_op_alt by insn code 2014-05-20 8:19 RFA: cache recog_op_alt by insn code Richard Sandiford @ 2014-05-20 18:04 ` Jeff Law 2014-05-26 19:21 ` Require '%' to be at the beginning of a constraint string Richard Sandiford 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford 0 siblings, 2 replies; 20+ messages in thread From: Jeff Law @ 2014-05-20 18:04 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/20/14 02:19, Richard Sandiford wrote: > Following on from (and depending on) the last patch, process_constraints > also shows up high in the profile. This patch caches the recog_op_alt > information by insn code too. It also shrinks the size of the structure > from 1 pointer + 5 ints to 1 pointer + 2 ints: > > - no target should have more than 65536 register classes :-) That could probably be much lower if we needed more bits for other things. > - "reject" is based on a cost of 600 for '!', so 16 bits should be plenty OK. Makes sense. > - "matched" and "matches" are operand numbers and so fit in 8 bits OK. This could also be smaller, don't we have an upper limit of 32 (or 30?) on the number of operands appearing in an insn. It'd be a way to get a few more bits if we need them someday. > Since this code is creating cached data and should only be run once > per insn code or asm statement, I don't think the extra in-loop > lra_static_insn_data assignments really justify having two copies > of such a complicated function. I think it would be better for LRA > to use the generic routine and then fill in the lra_static_insn_data > fields on the result (basically just an OR of "is_address" and > "early_clobber" for each alternative, plus setting up "commutative"). > We could then avoid having two caches of the same data. I'll do > that as a follow-up if it sounds OK. Seems reasonable. I suspect Vlad found the same code to be hot, hence the caching. Once he had a copy adding to it wasn't a big deal. But yes, I think that after the cache is globalized that having IRA walk over things another time shouldn't be a problem. > > On the subject of commutativity, we have: > > static bool > commutative_constraint_p (const char *str) > { > int curr_alt, c; > bool ignore_p; > > for (ignore_p = false, curr_alt = 0;;) > { > c = *str; > if (c == '\0') > break; > str += CONSTRAINT_LEN (c, str); > if (c == '#' || !recog_data.alternative_enabled_p[curr_alt]) > ignore_p = true; > else if (c == ',') > { > curr_alt++; > ignore_p = false; > } > else if (! ignore_p) > { > /* Usually `%' is the first constraint character but the > documentation does not require this. */ > if (c == '%') > return true; > } > } > return false; > } > > Any objections to requiring `%' to be the first constraint character? > Seems wasteful to be searching the constraint string just to support > an odd case. If we're going to change it, then clearly the docs need to change and ideally we'd statically check the port's constraints during the build process to ensure they meet the tighter definition. I'm sure David will be oh-so-happy to see state appearing. But that's something he'll have to deal with in the JIT side. > > The patch gives a further ~3.5% improvement in compile time for > -O0 fold-const.ii, on top of the other patch. > > Tested on x86_64-linux-gnu. OK to install? > > Thanks, > Richard > > > gcc/ > * recog.h (operand_alternative): Convert reg_class, reject, > matched and matches into bitfields. > (preprocess_constraints): Take the insn as parameter. > (recog_op_alt): Change into an array of pointers. > (target_recog): Add x_op_alt. > * recog.c (asm_op_alt_1, asm_op_alt): New variables > (recog_op_alt): Change into an array of pointers. > (preprocess_constraints): Update accordingly. Take the insn as > parameter. Use asm_op_alt_1 and asm_op_alt for asms. Cache other > instructions in this_target_recog. > * ira-lives.c (process_bb_node_lives): Pass the insn to > process_constraints. > * reg-stack.c (check_asm_stack_operands): Likewise. > (subst_asm_stack_regs): Likewise. > * regcprop.c (copyprop_hardreg_forward_1): Likewise. > * regrename.c (build_def_use): Likewise. > * sched-deps.c (sched_analyze_insn): Likewise. > * sel-sched.c (get_reg_class, implicit_clobber_conflict_p): Likewise. > * config/arm/arm.c (xscale_sched_adjust_cost): Likewise. > (note_invalid_constants): Likewise. > * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. OK. Thanks! Jeff > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Require '%' to be at the beginning of a constraint string 2014-05-20 18:04 ` Jeff Law @ 2014-05-26 19:21 ` Richard Sandiford 2014-05-27 17:41 ` Jeff Law 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford 1 sibling, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-26 19:21 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Jeff Law <law@redhat.com> writes: > On 05/20/14 02:19, Richard Sandiford wrote: >> On the subject of commutativity, we have: >> >> static bool >> commutative_constraint_p (const char *str) >> { >> int curr_alt, c; >> bool ignore_p; >> >> for (ignore_p = false, curr_alt = 0;;) >> { >> c = *str; >> if (c == '\0') >> break; >> str += CONSTRAINT_LEN (c, str); >> if (c == '#' || !recog_data.alternative_enabled_p[curr_alt]) >> ignore_p = true; >> else if (c == ',') >> { >> curr_alt++; >> ignore_p = false; >> } >> else if (! ignore_p) >> { >> /* Usually `%' is the first constraint character but the >> documentation does not require this. */ >> if (c == '%') >> return true; >> } >> } >> return false; >> } >> >> Any objections to requiring `%' to be the first constraint character? >> Seems wasteful to be searching the constraint string just to support >> an odd case. > If we're going to change it, then clearly the docs need to change and > ideally we'd statically check the port's constraints during the build > process to ensure they meet the tighter definition. OK, how does this look? I built a cc1 for one target per config/ directory to (try to) check that there were no remaining cases. This means that we will silently ignore '%'s that are embedded in the middle of an asm constraint string, but in a way that's already true for most places that check for commutativity. An error seems a bit extreme when '%' is only a hint. If we want a warning, what should the option be called? And should it be under -Wall, -Wextra, or on by default? Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * doc/md.texi: Document that the % constraint character must be at the beginning of the string. * genoutput.c (validate_insn_alternatives): Check that '=', '+' and '%' only appear at the beginning of a constraint. * ira.c (commutative_constraint_p): Delete. (ira_get_dup_out_num): Expect the '%' commutativity marker to be at the start of the string. * config/alpha/alpha.md (*movmemdi_1, *clrmemdi_1): Remove duplicate '='s. * config/arm/neon.md (bicdi3_neon): Likewise. * config/vax/vax.md (sbcdi3): Likewise. * config/h8300/h8300.md (*cmpstz): Remove duplicate '+'. * config/arc/arc.md (mulsi_600, mulsidi_600, umulsidi_600) (mul64): Move '%' to beginning of constraint. * config/arm/arm.md (*xordi3_insn): Likewise. * config/nds32/nds32.md (add<mode>3, mulsi3, andsi3, iorsi3) (xorsi3): Likewise. Index: gcc/doc/md.texi =================================================================== --- gcc/doc/md.texi 2014-05-26 19:47:27.560682653 +0100 +++ gcc/doc/md.texi 2014-05-26 20:08:23.808948838 +0100 @@ -1589,7 +1589,9 @@ See, for example, the @samp{mulsi3} insn Declares the instruction to be commutative for this operand and the following operand. This means that the compiler may interchange the two operands if that is the cheapest way to make all operands fit the -constraints. +constraints. @samp{%} applies to all alternatives and must appear as +the first character in the constraint. + @ifset INTERNALS This is often used in patterns for addition instructions that really have only two operands: the result must go in one of the Index: gcc/genoutput.c =================================================================== --- gcc/genoutput.c 2014-05-26 19:47:27.560682653 +0100 +++ gcc/genoutput.c 2014-05-26 20:08:23.675947636 +0100 @@ -781,6 +781,11 @@ validate_insn_alternatives (struct data for (p = d->operand[start].constraint; (c = *p); p += len) { + if ((c == '%' || c == '=' || c == '+') + && p != d->operand[start].constraint) + error_with_line (d->lineno, + "character '%c' can only be used at the" + " beginning of a constraint string"); #ifdef USE_MD_CONSTRAINTS if (ISSPACE (c) || strchr (indep_constraints, c)) len = 1; Index: gcc/ira.c =================================================================== --- gcc/ira.c 2014-05-26 20:10:28.988080881 +0100 +++ gcc/ira.c 2014-05-26 20:10:32.773115124 +0100 @@ -1769,38 +1769,6 @@ setup_prohibited_mode_move_regs (void) } \f - -/* Return TRUE if the operand constraint STR is commutative. */ -static bool -commutative_constraint_p (const char *str) -{ - int curr_alt, c; - bool ignore_p; - - for (ignore_p = false, curr_alt = 0;;) - { - c = *str; - if (c == '\0') - break; - str += CONSTRAINT_LEN (c, str); - if (c == '#' || !recog_data.alternative_enabled_p[curr_alt]) - ignore_p = true; - else if (c == ',') - { - curr_alt++; - ignore_p = false; - } - else if (! ignore_p) - { - /* Usually `%' is the first constraint character but the - documentation does not require this. */ - if (c == '%') - return true; - } - } - return false; -} - /* Setup possible alternatives in ALTS for INSN. */ void ira_setup_alts (rtx insn, HARD_REG_SET &alts) @@ -2101,10 +2069,9 @@ ira_get_dup_out_num (int op_num, HARD_RE if (use_commut_op_p) break; use_commut_op_p = true; - if (commutative_constraint_p (recog_data.constraints[op_num])) + if (recog_data.constraints[op_num][0] == '%') str = recog_data.constraints[op_num + 1]; - else if (op_num > 0 && commutative_constraint_p (recog_data.constraints - [op_num - 1])) + else if (op_num > 0 && recog_data.constraints[op_num - 1][0] == '%') str = recog_data.constraints[op_num - 1]; else break; Index: gcc/config/alpha/alpha.md =================================================================== --- gcc/config/alpha/alpha.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/alpha/alpha.md 2014-05-26 20:08:23.697947834 +0100 @@ -4764,7 +4764,7 @@ (define_expand "movmemdi" "operands[4] = gen_rtx_SYMBOL_REF (Pmode, \"OTS$MOVE\");") (define_insn "*movmemdi_1" - [(set (match_operand:BLK 0 "memory_operand" "=m,=m") + [(set (match_operand:BLK 0 "memory_operand" "=m,m") (match_operand:BLK 1 "memory_operand" "m,m")) (use (match_operand:DI 2 "nonmemory_operand" "r,i")) (use (match_operand:DI 3 "immediate_operand")) @@ -4831,7 +4831,7 @@ (define_expand "setmemdi" }) (define_insn "*clrmemdi_1" - [(set (match_operand:BLK 0 "memory_operand" "=m,=m") + [(set (match_operand:BLK 0 "memory_operand" "=m,m") (const_int 0)) (use (match_operand:DI 1 "nonmemory_operand" "r,i")) (use (match_operand:DI 2 "immediate_operand")) Index: gcc/config/arm/neon.md =================================================================== --- gcc/config/arm/neon.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/arm/neon.md 2014-05-26 20:08:23.720948042 +0100 @@ -728,7 +728,7 @@ (define_insn "bic<mode>3_neon" ;; Compare to *anddi_notdi_di. (define_insn "bicdi3_neon" - [(set (match_operand:DI 0 "s_register_operand" "=w,?=&r,?&r") + [(set (match_operand:DI 0 "s_register_operand" "=w,?&r,?&r") (and:DI (not:DI (match_operand:DI 2 "s_register_operand" "w,r,0")) (match_operand:DI 1 "s_register_operand" "w,0,r")))] "TARGET_NEON" Index: gcc/config/vax/vax.md =================================================================== --- gcc/config/vax/vax.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/vax/vax.md 2014-05-26 20:08:23.776948548 +0100 @@ -423,7 +423,7 @@ (define_expand "subdi3" "vax_expand_addsub_di_operands (operands, MINUS); DONE;") (define_insn "sbcdi3" - [(set (match_operand:DI 0 "nonimmediate_addsub_di_operand" "=Rr,=Rr") + [(set (match_operand:DI 0 "nonimmediate_addsub_di_operand" "=Rr,Rr") (minus:DI (match_operand:DI 1 "general_addsub_di_operand" "0,I") (match_operand:DI 2 "general_addsub_di_operand" "nRr,Rr")))] "TARGET_QMATH" Index: gcc/config/h8300/h8300.md =================================================================== --- gcc/config/h8300/h8300.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/h8300/h8300.md 2014-05-26 20:08:23.791948684 +0100 @@ -3589,7 +3589,7 @@ (define_insn "*bstzhireg" [(set_attr "cc" "clobber")]) (define_insn_and_split "*cmpstz" - [(set (zero_extract:QI (match_operand:QI 0 "bit_memory_operand" "+WU,+WU") + [(set (zero_extract:QI (match_operand:QI 0 "bit_memory_operand" "+WU,WU") (const_int 1) (match_operand:QI 1 "immediate_operand" "n,n")) (match_operator:QI 2 "eqne_operator" Index: gcc/config/arc/arc.md =================================================================== --- gcc/config/arc/arc.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/arc/arc.md 2014-05-26 20:08:23.590946867 +0100 @@ -1698,7 +1698,7 @@ (define_insn "mac_600" (define_insn "mulsi_600" [(set (match_operand:SI 2 "mlo_operand" "") - (mult:SI (match_operand:SI 0 "register_operand" "Rcq#q,c,c,%c") + (mult:SI (match_operand:SI 0 "register_operand" "%Rcq#q,c,c,c") (match_operand:SI 1 "nonmemory_operand" "Rcq#q,cL,I,Cal"))) (clobber (match_operand:SI 3 "mhi_operand" ""))] "TARGET_MUL64_SET" @@ -1750,7 +1750,7 @@ (define_insn "mulsi3_600_lib" (define_insn "mulsidi_600" [(set (reg:DI MUL64_OUT_REG) (mult:DI (sign_extend:DI - (match_operand:SI 0 "register_operand" "Rcq#q,c,c,%c")) + (match_operand:SI 0 "register_operand" "%Rcq#q,c,c,c")) (sign_extend:DI ; assembler issue for "I", see mulsi_600 ; (match_operand:SI 1 "register_operand" "Rcq#q,cL,I,Cal"))))] @@ -1766,7 +1766,7 @@ (define_insn "mulsidi_600" (define_insn "umulsidi_600" [(set (reg:DI MUL64_OUT_REG) (mult:DI (zero_extend:DI - (match_operand:SI 0 "register_operand" "c,c,%c")) + (match_operand:SI 0 "register_operand" "%c,c,c")) (sign_extend:DI ; assembler issue for "I", see mulsi_600 ; (match_operand:SI 1 "register_operand" "cL,I,Cal"))))] @@ -4134,7 +4134,7 @@ (define_insn "swap" ;; FIXME: an intrinsic for multiply is daft. Can we remove this? (define_insn "mul64" - [(unspec [(match_operand:SI 0 "general_operand" "q,r,r,%r") + [(unspec [(match_operand:SI 0 "general_operand" "%q,r,r,r") (match_operand:SI 1 "general_operand" "q,rL,I,Cal")] UNSPEC_MUL64)] "TARGET_MUL64_SET" Index: gcc/config/arm/arm.md =================================================================== --- gcc/config/arm/arm.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/arm/arm.md 2014-05-26 20:08:23.629947220 +0100 @@ -3169,7 +3169,7 @@ (define_expand "xordi3" (define_insn_and_split "*xordi3_insn" [(set (match_operand:DI 0 "s_register_operand" "=w,&r,&r,&r,&r,?w") - (xor:DI (match_operand:DI 1 "s_register_operand" "w ,%0,r ,0 ,r ,w") + (xor:DI (match_operand:DI 1 "s_register_operand" "%w ,0,r ,0 ,r ,w") (match_operand:DI 2 "arm_xordi_operand" "w ,r ,r ,Dg,Dg,w")))] "TARGET_32BIT && !TARGET_IWMMXT" { Index: gcc/config/nds32/nds32.md =================================================================== --- gcc/config/nds32/nds32.md 2014-05-26 19:47:27.560682653 +0100 +++ gcc/config/nds32/nds32.md 2014-05-26 20:08:23.663947527 +0100 @@ -261,7 +261,7 @@ (define_insn "extend<mode>si2" (define_insn "add<mode>3" [(set (match_operand:QIHISI 0 "register_operand" "= d, l, d, l, d, l, k, l, r, r") - (plus:QIHISI (match_operand:QIHISI 1 "register_operand" " 0, l, 0, l, %0, l, 0, k, r, r") + (plus:QIHISI (match_operand:QIHISI 1 "register_operand" "% 0, l, 0, l, 0, l, 0, k, r, r") (match_operand:QIHISI 2 "nds32_rimm15s_operand" " In05, In03, Iu05, Iu03, r, l, Is10, Iu06, Is15, r")))] "" { @@ -382,9 +382,9 @@ (define_insn "*sub_srli" ;; Multiplication instructions. (define_insn "mulsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r") - (mult:SI (match_operand:SI 1 "register_operand" " %0, r") - (match_operand:SI 2 "register_operand" " w, r")))] + [(set (match_operand:SI 0 "register_operand" "=w, r") + (mult:SI (match_operand:SI 1 "register_operand" "%0, r") + (match_operand:SI 2 "register_operand" " w, r")))] "" "@ mul33\t%0, %2 @@ -489,9 +489,9 @@ (define_insn "bitc" ) (define_insn "andsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r, l, l, l, l, l, l, r, r, r, r, r") - (and:SI (match_operand:SI 1 "register_operand" " %0, r, l, l, l, l, 0, 0, r, r, r, r, r") - (match_operand:SI 2 "general_operand" " w, r, Izeb, Izeh, Ixls, Ix11, Ibms, Ifex, Izeb, Izeh, Iu15, Ii15, Ic15")))] + [(set (match_operand:SI 0 "register_operand" "=w, r, l, l, l, l, l, l, r, r, r, r, r") + (and:SI (match_operand:SI 1 "register_operand" "%0, r, l, l, l, l, 0, 0, r, r, r, r, r") + (match_operand:SI 2 "general_operand" " w, r, Izeb, Izeh, Ixls, Ix11, Ibms, Ifex, Izeb, Izeh, Iu15, Ii15, Ic15")))] "" { HOST_WIDE_INT mask = INTVAL (operands[2]); @@ -585,9 +585,9 @@ (define_insn "*and_srli" ;; For V3/V3M ISA, we have 'or33' instruction. ;; So we can identify 'or Rt3,Rt3,Ra3' case and set its length to be 2. (define_insn "iorsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r, r, r") - (ior:SI (match_operand:SI 1 "register_operand" " %0, r, r, r") - (match_operand:SI 2 "general_operand" " w, r, Iu15, Ie15")))] + [(set (match_operand:SI 0 "register_operand" "=w, r, r, r") + (ior:SI (match_operand:SI 1 "register_operand" "%0, r, r, r") + (match_operand:SI 2 "general_operand" " w, r, Iu15, Ie15")))] "" { int one_position; @@ -645,9 +645,9 @@ (define_insn "*or_srli" ;; For V3/V3M ISA, we have 'xor33' instruction. ;; So we can identify 'xor Rt3,Rt3,Ra3' case and set its length to be 2. (define_insn "xorsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r, r, r") - (xor:SI (match_operand:SI 1 "register_operand" " %0, r, r, r") - (match_operand:SI 2 "general_operand" " w, r, Iu15, It15")))] + [(set (match_operand:SI 0 "register_operand" "=w, r, r, r") + (xor:SI (match_operand:SI 1 "register_operand" "%0, r, r, r") + (match_operand:SI 2 "general_operand" " w, r, Iu15, It15")))] "" { int one_position; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Require '%' to be at the beginning of a constraint string 2014-05-26 19:21 ` Require '%' to be at the beginning of a constraint string Richard Sandiford @ 2014-05-27 17:41 ` Jeff Law 2014-05-28 19:50 ` Richard Sandiford 0 siblings, 1 reply; 20+ messages in thread From: Jeff Law @ 2014-05-27 17:41 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/26/14 13:21, Richard Sandiford wrote: >> If we're going to change it, then clearly the docs need to change and >> ideally we'd statically check the port's constraints during the build >> process to ensure they meet the tighter definition. > > OK, how does this look? I built a cc1 for one target per config/ > directory to (try to) check that there were no remaining cases. > > This means that we will silently ignore '%'s that are embedded in the > middle of an asm constraint string, but in a way that's already true for > most places that check for commutativity. An error seems a bit extreme > when '%' is only a hint. If we want a warning, what should the option > be called? And should it be under -Wall, -Wextra, or on by default? > > Tested on x86_64-linux-gnu. OK to install? OK. My initial thought on adding a warning was to weed out bad constraints. You've already done that for the in-tree ports. I'm a lot less inclined to do much more here to help the out-of-tree ports, so upon further review, let's not worry about the warning unless you've already got it ready to go :-) > > Thanks, > Richard > > > gcc/ > * doc/md.texi: Document that the % constraint character must > be at the beginning of the string. > * genoutput.c (validate_insn_alternatives): Check that '=', > '+' and '%' only appear at the beginning of a constraint. > * ira.c (commutative_constraint_p): Delete. > (ira_get_dup_out_num): Expect the '%' commutativity marker to be > at the start of the string. > * config/alpha/alpha.md (*movmemdi_1, *clrmemdi_1): Remove > duplicate '='s. > * config/arm/neon.md (bicdi3_neon): Likewise. > * config/vax/vax.md (sbcdi3): Likewise. > * config/h8300/h8300.md (*cmpstz): Remove duplicate '+'. > * config/arc/arc.md (mulsi_600, mulsidi_600, umulsidi_600) > (mul64): Move '%' to beginning of constraint. > * config/arm/arm.md (*xordi3_insn): Likewise. > * config/nds32/nds32.md (add<mode>3, mulsi3, andsi3, iorsi3) > (xorsi3): Likewise. Those ARC port bits are odd. Why in the world would someone have the commutative modifier on the last (and only the last) alternative. Strange. Regardless, this is OK. Thanks, Jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Require '%' to be at the beginning of a constraint string 2014-05-27 17:41 ` Jeff Law @ 2014-05-28 19:50 ` Richard Sandiford 2014-05-30 16:24 ` Jeff Law 0 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-28 19:50 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Thanks for the review. Jeff Law <law@redhat.com> writes: > On 05/26/14 13:21, Richard Sandiford wrote: >>> If we're going to change it, then clearly the docs need to change and >>> ideally we'd statically check the port's constraints during the build >>> process to ensure they meet the tighter definition. >> >> OK, how does this look? I built a cc1 for one target per config/ >> directory to (try to) check that there were no remaining cases. >> >> This means that we will silently ignore '%'s that are embedded in the >> middle of an asm constraint string, but in a way that's already true for >> most places that check for commutativity. An error seems a bit extreme >> when '%' is only a hint. If we want a warning, what should the option >> be called? And should it be under -Wall, -Wextra, or on by default? >> >> Tested on x86_64-linux-gnu. OK to install? > OK. My initial thought on adding a warning was to weed out bad > constraints. You've already done that for the in-tree ports. I'm a lot > less inclined to do much more here to help the out-of-tree ports, so > upon further review, let's not worry about the warning unless you've > already got it ready to go :-) Well, the new genoutput.c error should catch problems in out-of-tree ports. It's just asms where the non-initial '%'s would be silently accepted and have no effect. David W suggested off-list that I add "Only input operands can use @samp{%}." to the documention as well. That seemed like it was obviously an improvement so I applied the patch with that change (below). Thanks, Richard gcc/ * doc/md.texi: Document that the % constraint character must be at the beginning of the string. * genoutput.c (validate_insn_alternatives): Check that '=', '+' and '%' only appear at the beginning of a constraint. * ira.c (commutative_constraint_p): Delete. (ira_get_dup_out_num): Expect the '%' commutativity marker to be at the start of the string. * config/alpha/alpha.md (*movmemdi_1, *clrmemdi_1): Remove duplicate '='s. * config/arm/neon.md (bicdi3_neon): Likewise. * config/iq2000/iq2000.md (addsi3_internal, subsi3_internal, sgt_si) (slt_si, sltu_si): Likewise. * config/vax/vax.md (sbcdi3): Likewise. * config/h8300/h8300.md (*cmpstz): Remove duplicate '+'. * config/arc/arc.md (mulsi_600, mulsidi_600, umulsidi_600) (mul64): Move '%' to beginning of constraint. * config/arm/arm.md (*xordi3_insn): Likewise. * config/nds32/nds32.md (add<mode>3, mulsi3, andsi3, iorsi3) (xorsi3): Likewise. Index: gcc/doc/md.texi =================================================================== --- gcc/doc/md.texi 2014-05-27 11:23:48.676099753 +0100 +++ gcc/doc/md.texi 2014-05-27 11:23:53.289138410 +0100 @@ -1589,7 +1589,10 @@ See, for example, the @samp{mulsi3} insn Declares the instruction to be commutative for this operand and the following operand. This means that the compiler may interchange the two operands if that is the cheapest way to make all operands fit the -constraints. +constraints. @samp{%} applies to all alternatives and must appear as +the first character in the constraint. Only input operands can use +@samp{%}. + @ifset INTERNALS This is often used in patterns for addition instructions that really have only two operands: the result must go in one of the Index: gcc/genoutput.c =================================================================== --- gcc/genoutput.c 2014-05-27 11:23:48.661099627 +0100 +++ gcc/genoutput.c 2014-05-27 11:23:53.284138368 +0100 @@ -781,6 +781,11 @@ validate_insn_alternatives (struct data for (p = d->operand[start].constraint; (c = *p); p += len) { + if ((c == '%' || c == '=' || c == '+') + && p != d->operand[start].constraint) + error_with_line (d->lineno, + "character '%c' can only be used at the" + " beginning of a constraint string", c); #ifdef USE_MD_CONSTRAINTS if (ISSPACE (c) || strchr (indep_constraints, c)) len = 1; Index: gcc/ira.c =================================================================== --- gcc/ira.c 2014-05-27 11:23:48.679099778 +0100 +++ gcc/ira.c 2014-05-27 11:24:23.746394430 +0100 @@ -1770,34 +1770,6 @@ setup_prohibited_mode_move_regs (void) \f -/* Return TRUE if the operand constraint STR is commutative. */ -static bool -commutative_constraint_p (const char *str) -{ - int c; - - alternative_mask enabled = recog_data.enabled_alternatives; - for (;;) - { - c = *str; - if (c == '\0') - break; - str += CONSTRAINT_LEN (c, str); - if (c == '#') - enabled &= ~ALTERNATIVE_BIT (0); - else if (c == ',') - enabled >>= 1; - else if (enabled & 1) - { - /* Usually `%' is the first constraint character but the - documentation does not require this. */ - if (c == '%') - return true; - } - } - return false; -} - /* Setup possible alternatives in ALTS for INSN. */ void ira_setup_alts (rtx insn, HARD_REG_SET &alts) @@ -2099,10 +2071,9 @@ ira_get_dup_out_num (int op_num, HARD_RE if (use_commut_op_p) break; use_commut_op_p = true; - if (commutative_constraint_p (recog_data.constraints[op_num])) + if (recog_data.constraints[op_num][0] == '%') str = recog_data.constraints[op_num + 1]; - else if (op_num > 0 && commutative_constraint_p (recog_data.constraints - [op_num - 1])) + else if (op_num > 0 && recog_data.constraints[op_num - 1][0] == '%') str = recog_data.constraints[op_num - 1]; else break; Index: gcc/config/alpha/alpha.md =================================================================== --- gcc/config/alpha/alpha.md 2014-05-27 11:23:48.663099644 +0100 +++ gcc/config/alpha/alpha.md 2014-05-27 11:23:53.285138376 +0100 @@ -4764,7 +4764,7 @@ (define_expand "movmemdi" "operands[4] = gen_rtx_SYMBOL_REF (Pmode, \"OTS$MOVE\");") (define_insn "*movmemdi_1" - [(set (match_operand:BLK 0 "memory_operand" "=m,=m") + [(set (match_operand:BLK 0 "memory_operand" "=m,m") (match_operand:BLK 1 "memory_operand" "m,m")) (use (match_operand:DI 2 "nonmemory_operand" "r,i")) (use (match_operand:DI 3 "immediate_operand")) @@ -4831,7 +4831,7 @@ (define_expand "setmemdi" }) (define_insn "*clrmemdi_1" - [(set (match_operand:BLK 0 "memory_operand" "=m,=m") + [(set (match_operand:BLK 0 "memory_operand" "=m,m") (const_int 0)) (use (match_operand:DI 1 "nonmemory_operand" "r,i")) (use (match_operand:DI 2 "immediate_operand")) Index: gcc/config/arm/neon.md =================================================================== --- gcc/config/arm/neon.md 2014-05-27 11:23:48.665099661 +0100 +++ gcc/config/arm/neon.md 2014-05-27 11:23:53.286138385 +0100 @@ -728,7 +728,7 @@ (define_insn "bic<mode>3_neon" ;; Compare to *anddi_notdi_di. (define_insn "bicdi3_neon" - [(set (match_operand:DI 0 "s_register_operand" "=w,?=&r,?&r") + [(set (match_operand:DI 0 "s_register_operand" "=w,?&r,?&r") (and:DI (not:DI (match_operand:DI 2 "s_register_operand" "w,r,0")) (match_operand:DI 1 "s_register_operand" "w,0,r")))] "TARGET_NEON" Index: gcc/config/iq2000/iq2000.md =================================================================== --- gcc/config/iq2000/iq2000.md 2014-05-27 11:23:48.681099795 +0100 +++ gcc/config/iq2000/iq2000.md 2014-05-27 11:23:53.292138435 +0100 @@ -260,7 +260,7 @@ (define_expand "addsi3" "") (define_insn "addsi3_internal" - [(set (match_operand:SI 0 "register_operand" "=d,=d") + [(set (match_operand:SI 0 "register_operand" "=d,d") (plus:SI (match_operand:SI 1 "reg_or_0_operand" "dJ,dJ") (match_operand:SI 2 "arith_operand" "d,I")))] "" @@ -286,7 +286,7 @@ (define_expand "subsi3" "") (define_insn "subsi3_internal" - [(set (match_operand:SI 0 "register_operand" "=d,=d") + [(set (match_operand:SI 0 "register_operand" "=d,d") (minus:SI (match_operand:SI 1 "reg_or_0_operand" "dJ,dJ") (match_operand:SI 2 "arith_operand" "d,I")))] "" @@ -1229,7 +1229,7 @@ (define_insn "sne_si_zero" (set_attr "mode" "SI")]) (define_insn "sgt_si" - [(set (match_operand:SI 0 "register_operand" "=d,=d") + [(set (match_operand:SI 0 "register_operand" "=d,d") (gt:SI (match_operand:SI 1 "register_operand" "d,d") (match_operand:SI 2 "reg_or_0_operand" "d,J")))] "" @@ -1240,7 +1240,7 @@ (define_insn "sgt_si" (set_attr "mode" "SI,SI")]) (define_insn "slt_si" - [(set (match_operand:SI 0 "register_operand" "=d,=d") + [(set (match_operand:SI 0 "register_operand" "=d,d") (lt:SI (match_operand:SI 1 "register_operand" "d,d") (match_operand:SI 2 "arith_operand" "d,I")))] "" @@ -1273,7 +1273,7 @@ (define_insn "sgtu_si" (set_attr "mode" "SI")]) (define_insn "sltu_si" - [(set (match_operand:SI 0 "register_operand" "=d,=d") + [(set (match_operand:SI 0 "register_operand" "=d,d") (ltu:SI (match_operand:SI 1 "register_operand" "d,d") (match_operand:SI 2 "arith_operand" "d,I")))] "" Index: gcc/config/vax/vax.md =================================================================== --- gcc/config/vax/vax.md 2014-05-27 11:23:48.666099669 +0100 +++ gcc/config/vax/vax.md 2014-05-27 11:23:53.286138385 +0100 @@ -423,7 +423,7 @@ (define_expand "subdi3" "vax_expand_addsub_di_operands (operands, MINUS); DONE;") (define_insn "sbcdi3" - [(set (match_operand:DI 0 "nonimmediate_addsub_di_operand" "=Rr,=Rr") + [(set (match_operand:DI 0 "nonimmediate_addsub_di_operand" "=Rr,Rr") (minus:DI (match_operand:DI 1 "general_addsub_di_operand" "0,I") (match_operand:DI 2 "general_addsub_di_operand" "nRr,Rr")))] "TARGET_QMATH" Index: gcc/config/h8300/h8300.md =================================================================== --- gcc/config/h8300/h8300.md 2014-05-27 11:23:48.668099686 +0100 +++ gcc/config/h8300/h8300.md 2014-05-27 11:23:53.288138401 +0100 @@ -3589,7 +3589,7 @@ (define_insn "*bstzhireg" [(set_attr "cc" "clobber")]) (define_insn_and_split "*cmpstz" - [(set (zero_extract:QI (match_operand:QI 0 "bit_memory_operand" "+WU,+WU") + [(set (zero_extract:QI (match_operand:QI 0 "bit_memory_operand" "+WU,WU") (const_int 1) (match_operand:QI 1 "immediate_operand" "n,n")) (match_operator:QI 2 "eqne_operator" Index: gcc/config/arc/arc.md =================================================================== --- gcc/config/arc/arc.md 2014-05-27 11:23:48.650099535 +0100 +++ gcc/config/arc/arc.md 2014-05-27 11:23:53.280138334 +0100 @@ -1698,7 +1698,7 @@ (define_insn "mac_600" (define_insn "mulsi_600" [(set (match_operand:SI 2 "mlo_operand" "") - (mult:SI (match_operand:SI 0 "register_operand" "Rcq#q,c,c,%c") + (mult:SI (match_operand:SI 0 "register_operand" "%Rcq#q,c,c,c") (match_operand:SI 1 "nonmemory_operand" "Rcq#q,cL,I,Cal"))) (clobber (match_operand:SI 3 "mhi_operand" ""))] "TARGET_MUL64_SET" @@ -1750,7 +1750,7 @@ (define_insn "mulsi3_600_lib" (define_insn "mulsidi_600" [(set (reg:DI MUL64_OUT_REG) (mult:DI (sign_extend:DI - (match_operand:SI 0 "register_operand" "Rcq#q,c,c,%c")) + (match_operand:SI 0 "register_operand" "%Rcq#q,c,c,c")) (sign_extend:DI ; assembler issue for "I", see mulsi_600 ; (match_operand:SI 1 "register_operand" "Rcq#q,cL,I,Cal"))))] @@ -1766,7 +1766,7 @@ (define_insn "mulsidi_600" (define_insn "umulsidi_600" [(set (reg:DI MUL64_OUT_REG) (mult:DI (zero_extend:DI - (match_operand:SI 0 "register_operand" "c,c,%c")) + (match_operand:SI 0 "register_operand" "%c,c,c")) (sign_extend:DI ; assembler issue for "I", see mulsi_600 ; (match_operand:SI 1 "register_operand" "cL,I,Cal"))))] @@ -4134,7 +4134,7 @@ (define_insn "swap" ;; FIXME: an intrinsic for multiply is daft. Can we remove this? (define_insn "mul64" - [(unspec [(match_operand:SI 0 "general_operand" "q,r,r,%r") + [(unspec [(match_operand:SI 0 "general_operand" "%q,r,r,r") (match_operand:SI 1 "general_operand" "q,rL,I,Cal")] UNSPEC_MUL64)] "TARGET_MUL64_SET" Index: gcc/config/arm/arm.md =================================================================== --- gcc/config/arm/arm.md 2014-05-27 11:23:48.654099569 +0100 +++ gcc/config/arm/arm.md 2014-05-27 11:23:53.282138351 +0100 @@ -3169,7 +3169,7 @@ (define_expand "xordi3" (define_insn_and_split "*xordi3_insn" [(set (match_operand:DI 0 "s_register_operand" "=w,&r,&r,&r,&r,?w") - (xor:DI (match_operand:DI 1 "s_register_operand" "w ,%0,r ,0 ,r ,w") + (xor:DI (match_operand:DI 1 "s_register_operand" "%w ,0,r ,0 ,r ,w") (match_operand:DI 2 "arm_xordi_operand" "w ,r ,r ,Dg,Dg,w")))] "TARGET_32BIT && !TARGET_IWMMXT" { Index: gcc/config/nds32/nds32.md =================================================================== --- gcc/config/nds32/nds32.md 2014-05-27 11:23:48.656099585 +0100 +++ gcc/config/nds32/nds32.md 2014-05-27 11:23:53.283138359 +0100 @@ -261,7 +261,7 @@ (define_insn "extend<mode>si2" (define_insn "add<mode>3" [(set (match_operand:QIHISI 0 "register_operand" "= d, l, d, l, d, l, k, l, r, r") - (plus:QIHISI (match_operand:QIHISI 1 "register_operand" " 0, l, 0, l, %0, l, 0, k, r, r") + (plus:QIHISI (match_operand:QIHISI 1 "register_operand" "% 0, l, 0, l, 0, l, 0, k, r, r") (match_operand:QIHISI 2 "nds32_rimm15s_operand" " In05, In03, Iu05, Iu03, r, l, Is10, Iu06, Is15, r")))] "" { @@ -382,9 +382,9 @@ (define_insn "*sub_srli" ;; Multiplication instructions. (define_insn "mulsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r") - (mult:SI (match_operand:SI 1 "register_operand" " %0, r") - (match_operand:SI 2 "register_operand" " w, r")))] + [(set (match_operand:SI 0 "register_operand" "=w, r") + (mult:SI (match_operand:SI 1 "register_operand" "%0, r") + (match_operand:SI 2 "register_operand" " w, r")))] "" "@ mul33\t%0, %2 @@ -489,9 +489,9 @@ (define_insn "bitc" ) (define_insn "andsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r, l, l, l, l, l, l, r, r, r, r, r") - (and:SI (match_operand:SI 1 "register_operand" " %0, r, l, l, l, l, 0, 0, r, r, r, r, r") - (match_operand:SI 2 "general_operand" " w, r, Izeb, Izeh, Ixls, Ix11, Ibms, Ifex, Izeb, Izeh, Iu15, Ii15, Ic15")))] + [(set (match_operand:SI 0 "register_operand" "=w, r, l, l, l, l, l, l, r, r, r, r, r") + (and:SI (match_operand:SI 1 "register_operand" "%0, r, l, l, l, l, 0, 0, r, r, r, r, r") + (match_operand:SI 2 "general_operand" " w, r, Izeb, Izeh, Ixls, Ix11, Ibms, Ifex, Izeb, Izeh, Iu15, Ii15, Ic15")))] "" { HOST_WIDE_INT mask = INTVAL (operands[2]); @@ -585,9 +585,9 @@ (define_insn "*and_srli" ;; For V3/V3M ISA, we have 'or33' instruction. ;; So we can identify 'or Rt3,Rt3,Ra3' case and set its length to be 2. (define_insn "iorsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r, r, r") - (ior:SI (match_operand:SI 1 "register_operand" " %0, r, r, r") - (match_operand:SI 2 "general_operand" " w, r, Iu15, Ie15")))] + [(set (match_operand:SI 0 "register_operand" "=w, r, r, r") + (ior:SI (match_operand:SI 1 "register_operand" "%0, r, r, r") + (match_operand:SI 2 "general_operand" " w, r, Iu15, Ie15")))] "" { int one_position; @@ -645,9 +645,9 @@ (define_insn "*or_srli" ;; For V3/V3M ISA, we have 'xor33' instruction. ;; So we can identify 'xor Rt3,Rt3,Ra3' case and set its length to be 2. (define_insn "xorsi3" - [(set (match_operand:SI 0 "register_operand" "= w, r, r, r") - (xor:SI (match_operand:SI 1 "register_operand" " %0, r, r, r") - (match_operand:SI 2 "general_operand" " w, r, Iu15, It15")))] + [(set (match_operand:SI 0 "register_operand" "=w, r, r, r") + (xor:SI (match_operand:SI 1 "register_operand" "%0, r, r, r") + (match_operand:SI 2 "general_operand" " w, r, Iu15, It15")))] "" { int one_position; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Require '%' to be at the beginning of a constraint string 2014-05-28 19:50 ` Richard Sandiford @ 2014-05-30 16:24 ` Jeff Law 0 siblings, 0 replies; 20+ messages in thread From: Jeff Law @ 2014-05-30 16:24 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/28/14 13:50, Richard Sandiford wrote: > Thanks for the review. > > Jeff Law <law@redhat.com> writes: >> On 05/26/14 13:21, Richard Sandiford wrote: >>>> If we're going to change it, then clearly the docs need to change and >>>> ideally we'd statically check the port's constraints during the build >>>> process to ensure they meet the tighter definition. >>> >>> OK, how does this look? I built a cc1 for one target per config/ >>> directory to (try to) check that there were no remaining cases. >>> >>> This means that we will silently ignore '%'s that are embedded in the >>> middle of an asm constraint string, but in a way that's already true for >>> most places that check for commutativity. An error seems a bit extreme >>> when '%' is only a hint. If we want a warning, what should the option >>> be called? And should it be under -Wall, -Wextra, or on by default? >>> >>> Tested on x86_64-linux-gnu. OK to install? >> OK. My initial thought on adding a warning was to weed out bad >> constraints. You've already done that for the in-tree ports. I'm a lot >> less inclined to do much more here to help the out-of-tree ports, so >> upon further review, let's not worry about the warning unless you've >> already got it ready to go :-) > > Well, the new genoutput.c error should catch problems in out-of-tree ports. Right. > It's just asms where the non-initial '%'s would be silently accepted > and have no effect. Right. And I believe that it's conservatively correct -- so we're OK here as well. > > David W suggested off-list that I add "Only input operands can use > @samp{%}." to the documention as well. That seemed like it was > obviously an improvement so I applied the patch with that change (below). Funny I was thinking about that when looking at the arc changes. I saw the '%' on operand0 and was confused thinking it made no sense to have a '%' for an output operand. Then I realized that 0/1 were inputs and 2 was the output. Thanks for pushing this through. jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 0/5] Cache recog_op_alt by insn code, take 2 2014-05-20 18:04 ` Jeff Law 2014-05-26 19:21 ` Require '%' to be at the beginning of a constraint string Richard Sandiford @ 2014-05-31 9:02 ` Richard Sandiford 2014-05-31 9:06 ` [PATCH 1/5] Flatten recog_op_alt and reorder entries Richard Sandiford ` (5 more replies) 1 sibling, 6 replies; 20+ messages in thread From: Richard Sandiford @ 2014-05-31 9:02 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Sorry Jeff, while working on the follow-on LRA patch I came across a couple of problems, so I need another round on this. First of all, I mentioned doing a follow-on patch to make LRA and recog use the same cache for operand_alternatives. I hadn't realised until I wrote that patch that there's a significant difference between the LRA and recog_op_alt versions of the information: recog_op_alt groups alternatives together by operand whereas LRA groups operands together by alternative. So in the cached flat array, recog_op_alt uses [op * n_alternatives + alt] whereas LRA uses [alt * n_operands + nop]. The two could only share a cache if they used the same order. I think the LRA ordering makes more sense. Quite a few places are only interested in alternative which_alternative, so grouping operands together by alternative gives a natural subarray. And there are places in IRA (which uses recog_op_alt rather than the LRA information) where the "for each alternative" loop is the outer one. A second difference was that preprocess_constraints skips disabled alternatives while LRA's setup_operand_alternative doesn't; LRA just checks for disabled alternatives when walking the array instead. That should make no difference in practice, but since the LRA approach would also make it easier to precompute the array at build time, I thought it would be a better way to go. It also gives a cleaner interface: we can have a preprocess_insn_constraints function that takes a (define_insn-based) insn and returns the operand_alternative information without any global state. Also, it turns out that sel-sched.c, regcprop.c and regrename.c modify the recog_op_alt structure as a convenient way of handling matching constraints. That obviously isn't a good idea if we want other passes to reuse the data later. I've fixed that and made the types "const" so that new instances shouldn't creep in. Also, I hadn't realised that a define_insn with no constraints at all has 0 alternatives rather than 1. Some passes nevertheless unconditionally access the recog_op_alt information for alternative which_alternative. I could have fixed that by making n_alternatives always be >= 1 in the static insn table. Several places do have fast paths for n_alternatives == 0 though, so I thought it would be better to handle the 0 alternative case in preprocess_constraints as well. All it needs is a MIN (..., 1). So the patch has now grown to 5 subpatches: 1. make recog_op_alt a flat array and order it in the same way as LRA 2. fix the places that modified recog_op_alt locally 3. don't handle the enabled attribute in preprocess_constraints and make sure all consumers check for disabled alternatives explicitly 4. the updated version of the original patch 5. get LRA to use the new cache too (the original follow-on patch) Jeff Law <law@redhat.com> writes: > On 05/20/14 02:19, Richard Sandiford wrote: >> Following on from (and depending on) the last patch, process_constraints >> also shows up high in the profile. This patch caches the recog_op_alt >> information by insn code too. It also shrinks the size of the structure >> from 1 pointer + 5 ints to 1 pointer + 2 ints: >> >> - no target should have more than 65536 register classes :-) > That could probably be much lower if we needed more bits for other things. > >> - "reject" is based on a cost of 600 for '!', so 16 bits should be plenty > OK. Makes sense. > > >> - "matched" and "matches" are operand numbers and so fit in 8 bits > OK. This could also be smaller, don't we have an upper limit of 32 (or > 30?) on the number of operands appearing in an insn. It'd be a way to > get a few more bits if we need them someday. Yeah, we could probably squeeze everything into a single int if we needed to and if we were prepared to have non-byte-sized integer fields. Going from 2 ints to 1 int wouldn't help LP64 hosts as things stand though, since we have a pointer at the beginning of the struct. Boostrapped & regression-tested on x86_64-linux-gnu. Also tested by building gcc.dg, g++.dg and gcc.c-torture for one target per config/ directory and making sure that there were no asm differences. Thanks, Richard ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/5] Flatten recog_op_alt and reorder entries 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford @ 2014-05-31 9:06 ` Richard Sandiford 2014-06-03 21:50 ` Jeff Law 2014-05-31 9:09 ` [PATCH 2/5] Don't modify recog_op_alt after preprocess_constraints Richard Sandiford ` (4 subsequent siblings) 5 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-31 9:06 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches As described in the covering note, this patch converts recog_op_alt into a flat array and orders the entries in the same way as the LRA cache. Several places just want the information for alternative which_alternative, so I added a convenience function for that. Thanks, Richard gcc/ * recog.h (recog_op_alt): Convert to a flat array. (which_op_alt): New function. * recog.c (recog_op_alt): Convert to a flat array. (preprocess_constraints): Update accordingly, grouping all operands of the same alternative together, rather than the other way around. * ira-lives.c (check_and_make_def_conflict): Likewise. (make_early_clobber_and_input_conflicts): Likewise. * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. * reg-stack.c (check_asm_stack_operands): Use which_op_alt. (subst_asm_stack_regs): Likewise. * regcprop.c (copyprop_hardreg_forward_1): Likewise. * regrename.c (hide_operands, record_out_operands): Likewise. (build_def_use): Likewise. * sel-sched.c (get_reg_class): Likewise. * config/arm/arm.c (note_invalid_constants): Likewise. Index: gcc/recog.h =================================================================== --- gcc/recog.h 2014-05-31 08:54:10.351219264 +0100 +++ gcc/recog.h 2014-05-31 08:57:21.608789337 +0100 @@ -256,9 +256,20 @@ struct recog_data_d extern struct recog_data_d recog_data; -/* Contains a vector of operand_alternative structures for every operand. - Set up by preprocess_constraints. */ -extern struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS][MAX_RECOG_ALTERNATIVES]; +extern struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS + * MAX_RECOG_ALTERNATIVES]; + +/* Return a pointer to an array in which index OP describes the constraints + on operand OP of the current instruction alternative (which_alternative). + Only valid after calling preprocess_constraints and constrain_operands. */ + +inline static operand_alternative * +which_op_alt () +{ + gcc_checking_assert (IN_RANGE (which_alternative, 0, + recog_data.n_alternatives - 1)); + return &recog_op_alt[which_alternative * recog_data.n_operands]; +} /* A table defined in insn-output.c that give information about each insn-code value. */ Index: gcc/recog.c =================================================================== --- gcc/recog.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/recog.c 2014-05-31 08:57:57.642085058 +0100 @@ -78,9 +78,11 @@ struct target_recog *this_target_recog = struct recog_data_d recog_data; -/* Contains a vector of operand_alternative structures for every operand. +/* Contains a vector of operand_alternative structures, such that + operand OP of alternative A is at index A * n_operands + OP. Set up by preprocess_constraints. */ -struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS][MAX_RECOG_ALTERNATIVES]; +struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS + * MAX_RECOG_ALTERNATIVES]; /* On return from `constrain_operands', indicate which alternative was satisfied. */ @@ -2330,24 +2332,25 @@ preprocess_constraints (void) { int i; - for (i = 0; i < recog_data.n_operands; i++) - memset (recog_op_alt[i], 0, (recog_data.n_alternatives - * sizeof (struct operand_alternative))); + int n_operands = recog_data.n_operands; + int n_alternatives = recog_data.n_alternatives; + int n_entries = n_operands * n_alternatives; + memset (recog_op_alt, 0, n_entries * sizeof (struct operand_alternative)); - for (i = 0; i < recog_data.n_operands; i++) + for (i = 0; i < n_operands; i++) { int j; struct operand_alternative *op_alt; const char *p = recog_data.constraints[i]; - op_alt = recog_op_alt[i]; + op_alt = recog_op_alt; - for (j = 0; j < recog_data.n_alternatives; j++) + for (j = 0; j < n_alternatives; j++, op_alt += n_operands) { - op_alt[j].cl = NO_REGS; - op_alt[j].constraint = p; - op_alt[j].matches = -1; - op_alt[j].matched = -1; + op_alt[i].cl = NO_REGS; + op_alt[i].constraint = p; + op_alt[i].matches = -1; + op_alt[i].matched = -1; if (!TEST_BIT (recog_data.enabled_alternatives, j)) { @@ -2357,7 +2360,7 @@ preprocess_constraints (void) if (*p == '\0' || *p == ',') { - op_alt[j].anything_ok = 1; + op_alt[i].anything_ok = 1; continue; } @@ -2385,77 +2388,77 @@ preprocess_constraints (void) break; case '?': - op_alt[j].reject += 6; + op_alt[i].reject += 6; break; case '!': - op_alt[j].reject += 600; + op_alt[i].reject += 600; break; case '&': - op_alt[j].earlyclobber = 1; + op_alt[i].earlyclobber = 1; break; case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9': { char *end; - op_alt[j].matches = strtoul (p, &end, 10); - recog_op_alt[op_alt[j].matches][j].matched = i; + op_alt[i].matches = strtoul (p, &end, 10); + op_alt[op_alt[i].matches].matched = i; p = end; } continue; case TARGET_MEM_CONSTRAINT: - op_alt[j].memory_ok = 1; + op_alt[i].memory_ok = 1; break; case '<': - op_alt[j].decmem_ok = 1; + op_alt[i].decmem_ok = 1; break; case '>': - op_alt[j].incmem_ok = 1; + op_alt[i].incmem_ok = 1; break; case 'V': - op_alt[j].nonoffmem_ok = 1; + op_alt[i].nonoffmem_ok = 1; break; case 'o': - op_alt[j].offmem_ok = 1; + op_alt[i].offmem_ok = 1; break; case 'X': - op_alt[j].anything_ok = 1; + op_alt[i].anything_ok = 1; break; case 'p': - op_alt[j].is_address = 1; - op_alt[j].cl = reg_class_subunion[(int) op_alt[j].cl] + op_alt[i].is_address = 1; + op_alt[i].cl = reg_class_subunion[(int) op_alt[i].cl] [(int) base_reg_class (VOIDmode, ADDR_SPACE_GENERIC, ADDRESS, SCRATCH)]; break; case 'g': case 'r': - op_alt[j].cl = - reg_class_subunion[(int) op_alt[j].cl][(int) GENERAL_REGS]; + op_alt[i].cl = + reg_class_subunion[(int) op_alt[i].cl][(int) GENERAL_REGS]; break; default: if (EXTRA_MEMORY_CONSTRAINT (c, p)) { - op_alt[j].memory_ok = 1; + op_alt[i].memory_ok = 1; break; } if (EXTRA_ADDRESS_CONSTRAINT (c, p)) { - op_alt[j].is_address = 1; - op_alt[j].cl + op_alt[i].is_address = 1; + op_alt[i].cl = (reg_class_subunion - [(int) op_alt[j].cl] + [(int) op_alt[i].cl] [(int) base_reg_class (VOIDmode, ADDR_SPACE_GENERIC, ADDRESS, SCRATCH)]); break; } - op_alt[j].cl + op_alt[i].cl = (reg_class_subunion - [(int) op_alt[j].cl] + [(int) op_alt[i].cl] [(int) REG_CLASS_FROM_CONSTRAINT ((unsigned char) c, p)]); break; } Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/ira-lives.c 2014-05-31 08:54:12.238234755 +0100 @@ -624,30 +624,35 @@ check_and_make_def_conflict (int alt, in advance_p = true; - for (use = 0; use < recog_data.n_operands; use++) + int n_operands = recog_data.n_operands; + operand_alternative *op_alt = &recog_op_alt[alt * n_operands]; + for (use = 0; use < n_operands; use++) { int alt1; if (use == def || recog_data.operand_type[use] == OP_OUT) continue; - if (recog_op_alt[use][alt].anything_ok) + if (op_alt[use].anything_ok) use_cl = ALL_REGS; else - use_cl = recog_op_alt[use][alt].cl; + use_cl = op_alt[use].cl; /* If there's any alternative that allows USE to match DEF, do not record a conflict. If that causes us to create an invalid instruction due to the earlyclobber, reload must fix it up. */ for (alt1 = 0; alt1 < recog_data.n_alternatives; alt1++) - if (recog_op_alt[use][alt1].matches == def - || (use < recog_data.n_operands - 1 - && recog_data.constraints[use][0] == '%' - && recog_op_alt[use + 1][alt1].matches == def) - || (use >= 1 - && recog_data.constraints[use - 1][0] == '%' - && recog_op_alt[use - 1][alt1].matches == def)) - break; + { + operand_alternative *op_alt1 = &recog_op_alt[alt1 * n_operands]; + if (op_alt1[use].matches == def + || (use < n_operands - 1 + && recog_data.constraints[use][0] == '%' + && op_alt1[use + 1].matches == def) + || (use >= 1 + && recog_data.constraints[use - 1][0] == '%' + && op_alt1[use - 1].matches == def)) + break; + } if (alt1 < recog_data.n_alternatives) continue; @@ -655,15 +660,15 @@ check_and_make_def_conflict (int alt, in advance_p = check_and_make_def_use_conflict (dreg, orig_dreg, def_cl, use, use_cl, advance_p); - if ((use_match = recog_op_alt[use][alt].matches) >= 0) + if ((use_match = op_alt[use].matches) >= 0) { if (use_match == def) continue; - if (recog_op_alt[use_match][alt].anything_ok) + if (op_alt[use_match].anything_ok) use_cl = ALL_REGS; else - use_cl = recog_op_alt[use_match][alt].cl; + use_cl = op_alt[use_match].cl; advance_p = check_and_make_def_use_conflict (dreg, orig_dreg, def_cl, use, use_cl, advance_p); } @@ -681,26 +686,29 @@ make_early_clobber_and_input_conflicts ( int def, def_match; enum reg_class def_cl; - for (alt = 0; alt < recog_data.n_alternatives; alt++) - for (def = 0; def < recog_data.n_operands; def++) + int n_alternatives = recog_data.n_alternatives; + int n_operands = recog_data.n_operands; + operand_alternative *op_alt = recog_op_alt; + for (alt = 0; alt < n_alternatives; alt++, op_alt += n_operands) + for (def = 0; def < n_operands; def++) { def_cl = NO_REGS; - if (recog_op_alt[def][alt].earlyclobber) + if (op_alt[def].earlyclobber) { - if (recog_op_alt[def][alt].anything_ok) + if (op_alt[def].anything_ok) def_cl = ALL_REGS; else - def_cl = recog_op_alt[def][alt].cl; + def_cl = op_alt[def].cl; check_and_make_def_conflict (alt, def, def_cl); } - if ((def_match = recog_op_alt[def][alt].matches) >= 0 - && (recog_op_alt[def_match][alt].earlyclobber - || recog_op_alt[def][alt].earlyclobber)) + if ((def_match = op_alt[def].matches) >= 0 + && (op_alt[def_match].earlyclobber + || op_alt[def].earlyclobber)) { - if (recog_op_alt[def_match][alt].anything_ok) + if (op_alt[def_match].anything_ok) def_cl = ALL_REGS; else - def_cl = recog_op_alt[def_match][alt].cl; + def_cl = op_alt[def_match].cl; check_and_make_def_conflict (alt, def, def_cl); } } Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/config/i386/i386.c 2014-05-31 08:54:12.236234739 +0100 @@ -5828,11 +5828,13 @@ ix86_legitimate_combined_insn (rtx insn) extract_insn (insn); preprocess_constraints (); - for (i = 0; i < recog_data.n_operands; i++) + int n_operands = recog_data.n_operands; + int n_alternatives = recog_data.n_alternatives; + for (i = 0; i < n_operands; i++) { rtx op = recog_data.operand[i]; enum machine_mode mode = GET_MODE (op); - struct operand_alternative *op_alt; + operand_alternative *op_alt; int offset = 0; bool win; int j; @@ -5867,19 +5869,19 @@ ix86_legitimate_combined_insn (rtx insn) if (!(REG_P (op) && HARD_REGISTER_P (op))) continue; - op_alt = recog_op_alt[i]; + op_alt = recog_op_alt; /* Operand has no constraints, anything is OK. */ - win = !recog_data.n_alternatives; + win = !n_alternatives; - for (j = 0; j < recog_data.n_alternatives; j++) + for (j = 0; j < n_alternatives; j++, op_alt += n_operands) { - if (op_alt[j].anything_ok - || (op_alt[j].matches != -1 + if (op_alt[i].anything_ok + || (op_alt[i].matches != -1 && operands_match_p (recog_data.operand[i], - recog_data.operand[op_alt[j].matches])) - || reg_fits_class_p (op, op_alt[j].cl, offset, mode)) + recog_data.operand[op_alt[i].matches])) + || reg_fits_class_p (op, op_alt[i].cl, offset, mode)) { win = true; break; Index: gcc/reg-stack.c =================================================================== --- gcc/reg-stack.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/reg-stack.c 2014-05-31 08:57:21.608789337 +0100 @@ -462,7 +462,6 @@ check_asm_stack_operands (rtx insn) char reg_used_as_output[FIRST_PSEUDO_REGISTER]; char implicitly_dies[FIRST_PSEUDO_REGISTER]; - int alt; rtx *clobber_reg = 0; int n_inputs, n_outputs; @@ -471,19 +470,19 @@ check_asm_stack_operands (rtx insn) alternative matches, this asm is malformed. */ extract_insn (insn); constrain_operands (1); - alt = which_alternative; preprocess_constraints (); get_asm_operands_in_out (body, &n_outputs, &n_inputs); - if (alt < 0) + if (which_alternative < 0) { malformed_asm = 1; /* Avoid further trouble with this insn. */ PATTERN (insn) = gen_rtx_USE (VOIDmode, const0_rtx); return 0; } + operand_alternative *op_alt = which_op_alt (); /* Strip SUBREGs here to make the following code simpler. */ for (i = 0; i < recog_data.n_operands; i++) @@ -527,7 +526,7 @@ check_asm_stack_operands (rtx insn) for (i = 0; i < n_outputs; i++) if (STACK_REG_P (recog_data.operand[i])) { - if (reg_class_size[(int) recog_op_alt[i][alt].cl] != 1) + if (reg_class_size[(int) op_alt[i].cl] != 1) { error_for_asm (insn, "output constraint %d must specify a single register", i); malformed_asm = 1; @@ -582,7 +581,7 @@ check_asm_stack_operands (rtx insn) if (operands_match_p (clobber_reg[j], recog_data.operand[i])) break; - if (j < n_clobbers || recog_op_alt[i][alt].matches >= 0) + if (j < n_clobbers || op_alt[i].matches >= 0) implicitly_dies[REGNO (recog_data.operand[i])] = 1; } @@ -610,7 +609,7 @@ check_asm_stack_operands (rtx insn) record any earlyclobber. */ for (i = n_outputs; i < n_outputs + n_inputs; i++) - if (recog_op_alt[i][alt].matches == -1) + if (op_alt[i].matches == -1) { int j; @@ -2006,7 +2005,6 @@ subst_stack_regs_pat (rtx insn, stack_pt subst_asm_stack_regs (rtx insn, stack_ptr regstack) { rtx body = PATTERN (insn); - int alt; rtx *note_reg; /* Array of note contents */ rtx **note_loc; /* Address of REG field of each note */ @@ -2030,14 +2028,12 @@ subst_asm_stack_regs (rtx insn, stack_pt such an insn in check_asm_stack_operands. */ extract_insn (insn); constrain_operands (1); - alt = which_alternative; preprocess_constraints (); + operand_alternative *op_alt = which_op_alt (); get_asm_operands_in_out (body, &n_outputs, &n_inputs); - gcc_assert (alt >= 0); - /* Strip SUBREGs here to make the following code simpler. */ for (i = 0; i < recog_data.n_operands; i++) if (GET_CODE (recog_data.operand[i]) == SUBREG @@ -2118,9 +2114,8 @@ subst_asm_stack_regs (rtx insn, stack_pt for (i = n_outputs; i < n_outputs + n_inputs; i++) if (STACK_REG_P (recog_data.operand[i]) - && reg_class_subset_p (recog_op_alt[i][alt].cl, - FLOAT_REGS) - && recog_op_alt[i][alt].cl != FLOAT_REGS) + && reg_class_subset_p (op_alt[i].cl, FLOAT_REGS) + && op_alt[i].cl != FLOAT_REGS) { /* If an operand needs to be in a particular reg in FLOAT_REGS, the constraint was either 't' or 'u'. Since @@ -2208,7 +2203,7 @@ subst_asm_stack_regs (rtx insn, stack_pt if (operands_match_p (clobber_reg[j], recog_data.operand[i])) break; - if (j < n_clobbers || recog_op_alt[i][alt].matches >= 0) + if (j < n_clobbers || op_alt[i].matches >= 0) { /* recog_data.operand[i] might not be at the top of stack. But that's OK, because all we need to do is pop the Index: gcc/regcprop.c =================================================================== --- gcc/regcprop.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/regcprop.c 2014-05-31 08:57:21.608789337 +0100 @@ -745,7 +745,7 @@ copyprop_hardreg_forward_1 (basic_block for (insn = BB_HEAD (bb); ; insn = NEXT_INSN (insn)) { - int n_ops, i, alt, predicated; + int n_ops, i, predicated; bool is_asm, any_replacements; rtx set; rtx link; @@ -775,7 +775,7 @@ copyprop_hardreg_forward_1 (basic_block if (! constrain_operands (1)) fatal_insn_not_found (insn); preprocess_constraints (); - alt = which_alternative; + operand_alternative *op_alt = which_op_alt (); n_ops = recog_data.n_operands; is_asm = asm_noperands (PATTERN (insn)) >= 0; @@ -786,10 +786,10 @@ copyprop_hardreg_forward_1 (basic_block predicated = GET_CODE (PATTERN (insn)) == COND_EXEC; for (i = 0; i < n_ops; ++i) { - int matches = recog_op_alt[i][alt].matches; + int matches = op_alt[i].matches; if (matches >= 0) - recog_op_alt[i][alt].cl = recog_op_alt[matches][alt].cl; - if (matches >= 0 || recog_op_alt[i][alt].matched >= 0 + op_alt[i].cl = op_alt[matches].cl; + if (matches >= 0 || op_alt[i].matched >= 0 || (predicated && recog_data.operand_type[i] == OP_OUT)) recog_data.operand_type[i] = OP_INOUT; } @@ -800,7 +800,7 @@ copyprop_hardreg_forward_1 (basic_block /* For each earlyclobber operand, zap the value data. */ for (i = 0; i < n_ops; i++) - if (recog_op_alt[i][alt].earlyclobber) + if (op_alt[i].earlyclobber) kill_value (recog_data.operand[i], vd); /* Within asms, a clobber cannot overlap inputs or outputs. @@ -814,7 +814,7 @@ copyprop_hardreg_forward_1 (basic_block /* Kill all early-clobbered operands. */ for (i = 0; i < n_ops; i++) - if (recog_op_alt[i][alt].earlyclobber) + if (op_alt[i].earlyclobber) kill_value (recog_data.operand[i], vd); /* If we have dead sets in the insn, then we need to note these as we @@ -936,17 +936,15 @@ copyprop_hardreg_forward_1 (basic_block if (recog_data.operand_type[i] == OP_IN) { - if (recog_op_alt[i][alt].is_address) + if (op_alt[i].is_address) replaced[i] = replace_oldest_value_addr (recog_data.operand_loc[i], - recog_op_alt[i][alt].cl, - VOIDmode, ADDR_SPACE_GENERIC, - insn, vd); + op_alt[i].cl, VOIDmode, + ADDR_SPACE_GENERIC, insn, vd); else if (REG_P (recog_data.operand[i])) replaced[i] = replace_oldest_value_reg (recog_data.operand_loc[i], - recog_op_alt[i][alt].cl, - insn, vd); + op_alt[i].cl, insn, vd); else if (MEM_P (recog_data.operand[i])) replaced[i] = replace_oldest_value_mem (recog_data.operand[i], insn, vd); Index: gcc/regrename.c =================================================================== --- gcc/regrename.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/regrename.c 2014-05-31 08:57:21.608789337 +0100 @@ -1427,7 +1427,7 @@ hide_operands (int n_ops, rtx *old_opera unsigned HOST_WIDE_INT do_not_hide, bool inout_and_ec_only) { int i; - int alt = which_alternative; + operand_alternative *op_alt = which_op_alt (); for (i = 0; i < n_ops; i++) { old_operands[i] = recog_data.operand[i]; @@ -1439,7 +1439,7 @@ hide_operands (int n_ops, rtx *old_opera if (do_not_hide & (1 << i)) continue; if (!inout_and_ec_only || recog_data.operand_type[i] == OP_INOUT - || recog_op_alt[i][alt].earlyclobber) + || op_alt[i].earlyclobber) *recog_data.operand_loc[i] = cc0_rtx; } for (i = 0; i < recog_data.n_dups; i++) @@ -1449,7 +1449,7 @@ hide_operands (int n_ops, rtx *old_opera if (do_not_hide & (1 << opn)) continue; if (!inout_and_ec_only || recog_data.operand_type[opn] == OP_INOUT - || recog_op_alt[opn][alt].earlyclobber) + || op_alt[opn].earlyclobber) *recog_data.dup_loc[i] = cc0_rtx; } } @@ -1478,7 +1478,7 @@ restore_operands (rtx insn, int n_ops, r record_out_operands (rtx insn, bool earlyclobber, insn_rr_info *insn_info) { int n_ops = recog_data.n_operands; - int alt = which_alternative; + operand_alternative *op_alt = which_op_alt (); int i; @@ -1489,12 +1489,12 @@ record_out_operands (rtx insn, bool earl ? recog_data.operand_loc[opn] : recog_data.dup_loc[i - n_ops]); rtx op = *loc; - enum reg_class cl = recog_op_alt[opn][alt].cl; + enum reg_class cl = op_alt[opn].cl; struct du_head *prev_open; if (recog_data.operand_type[opn] != OP_OUT - || recog_op_alt[opn][alt].earlyclobber != earlyclobber) + || op_alt[opn].earlyclobber != earlyclobber) continue; if (insn_info) @@ -1539,7 +1539,6 @@ build_def_use (basic_block bb) rtx old_operands[MAX_RECOG_OPERANDS]; rtx old_dups[MAX_DUP_OPERANDS]; int i; - int alt; int predicated; enum rtx_code set_code = SET; enum rtx_code clobber_code = CLOBBER; @@ -1572,7 +1571,7 @@ build_def_use (basic_block bb) if (! constrain_operands (1)) fatal_insn_not_found (insn); preprocess_constraints (); - alt = which_alternative; + operand_alternative *op_alt = which_op_alt (); n_ops = recog_data.n_operands; untracked_operands = 0; @@ -1595,10 +1594,10 @@ build_def_use (basic_block bb) for (i = 0; i < n_ops; ++i) { rtx op = recog_data.operand[i]; - int matches = recog_op_alt[i][alt].matches; + int matches = op_alt[i].matches; if (matches >= 0) - recog_op_alt[i][alt].cl = recog_op_alt[matches][alt].cl; - if (matches >= 0 || recog_op_alt[i][alt].matched >= 0 + op_alt[i].cl = op_alt[matches].cl; + if (matches >= 0 || op_alt[i].matched >= 0 || (predicated && recog_data.operand_type[i] == OP_OUT)) { recog_data.operand_type[i] = OP_INOUT; @@ -1682,7 +1681,7 @@ build_def_use (basic_block bb) rtx *loc = (i < n_ops ? recog_data.operand_loc[opn] : recog_data.dup_loc[i - n_ops]); - enum reg_class cl = recog_op_alt[opn][alt].cl; + enum reg_class cl = op_alt[opn].cl; enum op_type type = recog_data.operand_type[opn]; /* Don't scan match_operand here, since we've no reg class @@ -1694,7 +1693,7 @@ build_def_use (basic_block bb) if (insn_info) cur_operand = i == opn ? insn_info->op_info + i : NULL; - if (recog_op_alt[opn][alt].is_address) + if (op_alt[opn].is_address) scan_rtx_address (insn, loc, cl, mark_read, VOIDmode, ADDR_SPACE_GENERIC); else Index: gcc/sel-sched.c =================================================================== --- gcc/sel-sched.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/sel-sched.c 2014-05-31 08:57:21.608789337 +0100 @@ -1014,20 +1014,20 @@ vinsn_writes_one_of_regs_p (vinsn_t vi, static enum reg_class get_reg_class (rtx insn) { - int alt, i, n_ops; + int i, n_ops; extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); preprocess_constraints (); - alt = which_alternative; n_ops = recog_data.n_operands; + operand_alternative *op_alt = which_op_alt (); for (i = 0; i < n_ops; ++i) { - int matches = recog_op_alt[i][alt].matches; + int matches = op_alt[i].matches; if (matches >= 0) - recog_op_alt[i][alt].cl = recog_op_alt[matches][alt].cl; + op_alt[i].cl = op_alt[matches].cl; } if (asm_noperands (PATTERN (insn)) > 0) @@ -1037,7 +1037,7 @@ get_reg_class (rtx insn) { rtx *loc = recog_data.operand_loc[i]; rtx op = *loc; - enum reg_class cl = recog_op_alt[i][alt].cl; + enum reg_class cl = op_alt[i].cl; if (REG_P (op) && REGNO (op) == ORIGINAL_REGNO (op)) @@ -1051,7 +1051,7 @@ get_reg_class (rtx insn) for (i = 0; i < n_ops + recog_data.n_dups; i++) { int opn = i < n_ops ? i : recog_data.dup_num[i - n_ops]; - enum reg_class cl = recog_op_alt[opn][alt].cl; + enum reg_class cl = op_alt[opn].cl; if (recog_data.operand_type[opn] == OP_OUT || recog_data.operand_type[opn] == OP_INOUT) Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c 2014-05-31 08:54:10.351219264 +0100 +++ gcc/config/arm/arm.c 2014-05-31 08:57:21.608789337 +0100 @@ -16872,6 +16872,7 @@ note_invalid_constants (rtx insn, HOST_W this insn. */ preprocess_constraints (); + operand_alternative *op_alt = which_op_alt (); for (opno = 0; opno < recog_data.n_operands; opno++) { /* Things we need to fix can only occur in inputs. */ @@ -16882,7 +16883,7 @@ note_invalid_constants (rtx insn, HOST_W of constants in this alternative is really to fool reload into allowing us to accept one there. We need to fix them up now so that we output the right code. */ - if (recog_op_alt[opno][which_alternative].memory_ok) + if (op_alt[opno].memory_ok) { rtx op = recog_data.operand[opno]; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/5] Flatten recog_op_alt and reorder entries 2014-05-31 9:06 ` [PATCH 1/5] Flatten recog_op_alt and reorder entries Richard Sandiford @ 2014-06-03 21:50 ` Jeff Law 0 siblings, 0 replies; 20+ messages in thread From: Jeff Law @ 2014-06-03 21:50 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/31/14 03:06, Richard Sandiford wrote: > As described in the covering note, this patch converts recog_op_alt > into a flat array and orders the entries in the same way as the > LRA cache. Several places just want the information for alternative > which_alternative, so I added a convenience function for that. > > Thanks, > Richard > > > gcc/ > * recog.h (recog_op_alt): Convert to a flat array. > (which_op_alt): New function. > * recog.c (recog_op_alt): Convert to a flat array. > (preprocess_constraints): Update accordingly, grouping all > operands of the same alternative together, rather than the > other way around. > * ira-lives.c (check_and_make_def_conflict): Likewise. > (make_early_clobber_and_input_conflicts): Likewise. > * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. > * reg-stack.c (check_asm_stack_operands): Use which_op_alt. > (subst_asm_stack_regs): Likewise. > * regcprop.c (copyprop_hardreg_forward_1): Likewise. > * regrename.c (hide_operands, record_out_operands): Likewise. > (build_def_use): Likewise. > * sel-sched.c (get_reg_class): Likewise. > * config/arm/arm.c (note_invalid_constants): Likewise. Largely mechanical. Looks good to me. Jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/5] Don't modify recog_op_alt after preprocess_constraints 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford 2014-05-31 9:06 ` [PATCH 1/5] Flatten recog_op_alt and reorder entries Richard Sandiford @ 2014-05-31 9:09 ` Richard Sandiford 2014-06-03 21:53 ` Jeff Law 2014-05-31 9:15 ` [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute Richard Sandiford ` (3 subsequent siblings) 5 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-31 9:09 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Since the aim of this series is to cache the result of preprocess_constraints, we need to make sure that passes don't modify the information afterwards. This patch deals with the places that did. Patch 4 will make the information properly const. Thanks, Richard gcc/ * recog.h (alternative_class): New function. (which_op_alt): Return a const recog_op_alt. * reg-stack.c (check_asm_stack_operands): Update type accordingly. (subst_asm_stack_regs): Likewise. * config/arm/arm.c (note_invalid_constants): Likewise. * regcprop.c (copyprop_hardreg_forward_1): Likewise. Don't modify the operand_alternative; use alternative class instead. * sel-sched.c (get_reg_class): Likewise. * regrename.c (build_def_use): Likewise. (hide_operands, restore_operands, record_out_operands): Update type accordingly. Index: gcc/recog.h =================================================================== --- gcc/recog.h 2014-05-31 08:57:21.608789337 +0100 +++ gcc/recog.h 2014-05-31 09:02:35.956369716 +0100 @@ -79,6 +79,14 @@ struct operand_alternative unsigned int anything_ok:1; }; +/* Return the class for operand I of alternative ALT, taking matching + constraints into account. */ + +static inline enum reg_class +alternative_class (const operand_alternative *alt, int i) +{ + return alt[i].matches >= 0 ? alt[alt[i].matches].cl : alt[i].cl; +} extern void init_recog (void); extern void init_recog_no_volatile (void); @@ -263,7 +271,7 @@ struct recog_data_d on operand OP of the current instruction alternative (which_alternative). Only valid after calling preprocess_constraints and constrain_operands. */ -inline static operand_alternative * +inline static const operand_alternative * which_op_alt () { gcc_checking_assert (IN_RANGE (which_alternative, 0, Index: gcc/reg-stack.c =================================================================== --- gcc/reg-stack.c 2014-05-31 08:57:21.608789337 +0100 +++ gcc/reg-stack.c 2014-05-31 09:02:35.968369814 +0100 @@ -482,7 +482,7 @@ check_asm_stack_operands (rtx insn) PATTERN (insn) = gen_rtx_USE (VOIDmode, const0_rtx); return 0; } - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); /* Strip SUBREGs here to make the following code simpler. */ for (i = 0; i < recog_data.n_operands; i++) @@ -2030,7 +2030,7 @@ subst_asm_stack_regs (rtx insn, stack_pt constrain_operands (1); preprocess_constraints (); - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); get_asm_operands_in_out (body, &n_outputs, &n_inputs); Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c 2014-05-31 08:57:21.608789337 +0100 +++ gcc/config/arm/arm.c 2014-05-31 09:02:35.966369798 +0100 @@ -16872,7 +16872,7 @@ note_invalid_constants (rtx insn, HOST_W this insn. */ preprocess_constraints (); - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); for (opno = 0; opno < recog_data.n_operands; opno++) { /* Things we need to fix can only occur in inputs. */ Index: gcc/regcprop.c =================================================================== --- gcc/regcprop.c 2014-05-31 08:57:21.608789337 +0100 +++ gcc/regcprop.c 2014-05-31 09:02:35.957369724 +0100 @@ -775,20 +775,17 @@ copyprop_hardreg_forward_1 (basic_block if (! constrain_operands (1)) fatal_insn_not_found (insn); preprocess_constraints (); - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); n_ops = recog_data.n_operands; is_asm = asm_noperands (PATTERN (insn)) >= 0; - /* Simplify the code below by rewriting things to reflect - matching constraints. Also promote OP_OUT to OP_INOUT + /* Simplify the code below by promoting OP_OUT to OP_INOUT in predicated instructions. */ predicated = GET_CODE (PATTERN (insn)) == COND_EXEC; for (i = 0; i < n_ops; ++i) { int matches = op_alt[i].matches; - if (matches >= 0) - op_alt[i].cl = op_alt[matches].cl; if (matches >= 0 || op_alt[i].matched >= 0 || (predicated && recog_data.operand_type[i] == OP_OUT)) recog_data.operand_type[i] = OP_INOUT; @@ -939,12 +936,14 @@ copyprop_hardreg_forward_1 (basic_block if (op_alt[i].is_address) replaced[i] = replace_oldest_value_addr (recog_data.operand_loc[i], - op_alt[i].cl, VOIDmode, - ADDR_SPACE_GENERIC, insn, vd); + alternative_class (op_alt, i), + VOIDmode, ADDR_SPACE_GENERIC, + insn, vd); else if (REG_P (recog_data.operand[i])) replaced[i] = replace_oldest_value_reg (recog_data.operand_loc[i], - op_alt[i].cl, insn, vd); + alternative_class (op_alt, i), + insn, vd); else if (MEM_P (recog_data.operand[i])) replaced[i] = replace_oldest_value_mem (recog_data.operand[i], insn, vd); Index: gcc/sel-sched.c =================================================================== --- gcc/sel-sched.c 2014-05-31 08:57:21.608789337 +0100 +++ gcc/sel-sched.c 2014-05-31 09:02:35.970369830 +0100 @@ -1022,14 +1022,7 @@ get_reg_class (rtx insn) preprocess_constraints (); n_ops = recog_data.n_operands; - operand_alternative *op_alt = which_op_alt (); - for (i = 0; i < n_ops; ++i) - { - int matches = op_alt[i].matches; - if (matches >= 0) - op_alt[i].cl = op_alt[matches].cl; - } - + const operand_alternative *op_alt = which_op_alt (); if (asm_noperands (PATTERN (insn)) > 0) { for (i = 0; i < n_ops; i++) @@ -1037,7 +1030,7 @@ get_reg_class (rtx insn) { rtx *loc = recog_data.operand_loc[i]; rtx op = *loc; - enum reg_class cl = op_alt[i].cl; + enum reg_class cl = alternative_class (op_alt, i); if (REG_P (op) && REGNO (op) == ORIGINAL_REGNO (op)) @@ -1051,7 +1044,7 @@ get_reg_class (rtx insn) for (i = 0; i < n_ops + recog_data.n_dups; i++) { int opn = i < n_ops ? i : recog_data.dup_num[i - n_ops]; - enum reg_class cl = op_alt[opn].cl; + enum reg_class cl = alternative_class (op_alt, opn); if (recog_data.operand_type[opn] == OP_OUT || recog_data.operand_type[opn] == OP_INOUT) Index: gcc/regrename.c =================================================================== --- gcc/regrename.c 2014-05-31 08:57:21.608789337 +0100 +++ gcc/regrename.c 2014-05-31 09:02:35.958369732 +0100 @@ -1427,7 +1427,7 @@ hide_operands (int n_ops, rtx *old_opera unsigned HOST_WIDE_INT do_not_hide, bool inout_and_ec_only) { int i; - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); for (i = 0; i < n_ops; i++) { old_operands[i] = recog_data.operand[i]; @@ -1478,7 +1478,7 @@ restore_operands (rtx insn, int n_ops, r record_out_operands (rtx insn, bool earlyclobber, insn_rr_info *insn_info) { int n_ops = recog_data.n_operands; - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); int i; @@ -1489,7 +1489,7 @@ record_out_operands (rtx insn, bool earl ? recog_data.operand_loc[opn] : recog_data.dup_loc[i - n_ops]); rtx op = *loc; - enum reg_class cl = op_alt[opn].cl; + enum reg_class cl = alternative_class (op_alt, opn); struct du_head *prev_open; @@ -1571,7 +1571,7 @@ build_def_use (basic_block bb) if (! constrain_operands (1)) fatal_insn_not_found (insn); preprocess_constraints (); - operand_alternative *op_alt = which_op_alt (); + const operand_alternative *op_alt = which_op_alt (); n_ops = recog_data.n_operands; untracked_operands = 0; @@ -1584,8 +1584,7 @@ build_def_use (basic_block bb) sizeof (operand_rr_info) * recog_data.n_operands); } - /* Simplify the code below by rewriting things to reflect - matching constraints. Also promote OP_OUT to OP_INOUT in + /* Simplify the code below by promoting OP_OUT to OP_INOUT in predicated instructions, but only for register operands that are already tracked, so that we can create a chain when the first SET makes a register live. */ @@ -1595,8 +1594,6 @@ build_def_use (basic_block bb) { rtx op = recog_data.operand[i]; int matches = op_alt[i].matches; - if (matches >= 0) - op_alt[i].cl = op_alt[matches].cl; if (matches >= 0 || op_alt[i].matched >= 0 || (predicated && recog_data.operand_type[i] == OP_OUT)) { @@ -1681,7 +1678,7 @@ build_def_use (basic_block bb) rtx *loc = (i < n_ops ? recog_data.operand_loc[opn] : recog_data.dup_loc[i - n_ops]); - enum reg_class cl = op_alt[opn].cl; + enum reg_class cl = alternative_class (op_alt, opn); enum op_type type = recog_data.operand_type[opn]; /* Don't scan match_operand here, since we've no reg class ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/5] Don't modify recog_op_alt after preprocess_constraints 2014-05-31 9:09 ` [PATCH 2/5] Don't modify recog_op_alt after preprocess_constraints Richard Sandiford @ 2014-06-03 21:53 ` Jeff Law 0 siblings, 0 replies; 20+ messages in thread From: Jeff Law @ 2014-06-03 21:53 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/31/14 03:09, Richard Sandiford wrote: > Since the aim of this series is to cache the result of preprocess_constraints, > we need to make sure that passes don't modify the information afterwards. > This patch deals with the places that did. Patch 4 will make the information > properly const. > > Thanks, > Richard > > > gcc/ > * recog.h (alternative_class): New function. > (which_op_alt): Return a const recog_op_alt. > * reg-stack.c (check_asm_stack_operands): Update type accordingly. > (subst_asm_stack_regs): Likewise. > * config/arm/arm.c (note_invalid_constants): Likewise. > * regcprop.c (copyprop_hardreg_forward_1): Likewise. Don't modify > the operand_alternative; use alternative class instead. > * sel-sched.c (get_reg_class): Likewise. > * regrename.c (build_def_use): Likewise. > (hide_operands, restore_operands, record_out_operands): Update type > accordingly. OK. Jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford 2014-05-31 9:06 ` [PATCH 1/5] Flatten recog_op_alt and reorder entries Richard Sandiford 2014-05-31 9:09 ` [PATCH 2/5] Don't modify recog_op_alt after preprocess_constraints Richard Sandiford @ 2014-05-31 9:15 ` Richard Sandiford 2014-06-03 21:58 ` Jeff Law 2014-05-31 9:17 ` [PATCH 4/5] Cache recog_op_alt by insn code: main patch Richard Sandiford ` (2 subsequent siblings) 5 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-31 9:15 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches As described in the covering note, it seems better to put the onus of checking the enabled attribute on the passes that are walking each alternative, like LRA does for its internal subpasses. That leads to a nicer interface in patch 4 and would make it easier to precompute the information at build time. (The only thing preventing that now is the subunion class.) Thanks, Richard gcc/ * recog.c (preprocess_constraints): Don't skip disabled alternatives. * ira-lives.c (check_and_make_def_conflict): Check for disabled alternatives. (make_early_clobber_and_input_conflicts): Likewise. * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. Index: gcc/recog.c =================================================================== --- gcc/recog.c 2014-05-31 08:57:57.642085058 +0100 +++ gcc/recog.c 2014-05-31 09:07:21.669714878 +0100 @@ -2352,12 +2352,6 @@ preprocess_constraints (void) op_alt[i].matches = -1; op_alt[i].matched = -1; - if (!TEST_BIT (recog_data.enabled_alternatives, j)) - { - p = skip_alternative (p); - continue; - } - if (*p == '\0' || *p == ',') { op_alt[i].anything_ok = 1; Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2014-05-31 08:54:12.238234755 +0100 +++ gcc/ira-lives.c 2014-05-31 09:07:21.670714886 +0100 @@ -641,8 +641,11 @@ check_and_make_def_conflict (int alt, in /* If there's any alternative that allows USE to match DEF, do not record a conflict. If that causes us to create an invalid instruction due to the earlyclobber, reload must fix it up. */ + alternative_mask enabled = recog_data.enabled_alternatives; for (alt1 = 0; alt1 < recog_data.n_alternatives; alt1++) { + if (!TEST_BIT (enabled, alt1)) + continue; operand_alternative *op_alt1 = &recog_op_alt[alt1 * n_operands]; if (op_alt1[use].matches == def || (use < n_operands - 1 @@ -688,30 +691,32 @@ make_early_clobber_and_input_conflicts ( int n_alternatives = recog_data.n_alternatives; int n_operands = recog_data.n_operands; + alternative_mask enabled = recog_data.enabled_alternatives; operand_alternative *op_alt = recog_op_alt; for (alt = 0; alt < n_alternatives; alt++, op_alt += n_operands) - for (def = 0; def < n_operands; def++) - { - def_cl = NO_REGS; - if (op_alt[def].earlyclobber) - { - if (op_alt[def].anything_ok) - def_cl = ALL_REGS; - else - def_cl = op_alt[def].cl; - check_and_make_def_conflict (alt, def, def_cl); - } - if ((def_match = op_alt[def].matches) >= 0 - && (op_alt[def_match].earlyclobber - || op_alt[def].earlyclobber)) - { - if (op_alt[def_match].anything_ok) - def_cl = ALL_REGS; - else - def_cl = op_alt[def_match].cl; - check_and_make_def_conflict (alt, def, def_cl); - } - } + if (TEST_BIT (enabled, alt)) + for (def = 0; def < n_operands; def++) + { + def_cl = NO_REGS; + if (op_alt[def].earlyclobber) + { + if (op_alt[def].anything_ok) + def_cl = ALL_REGS; + else + def_cl = op_alt[def].cl; + check_and_make_def_conflict (alt, def, def_cl); + } + if ((def_match = op_alt[def].matches) >= 0 + && (op_alt[def_match].earlyclobber + || op_alt[def].earlyclobber)) + { + if (op_alt[def_match].anything_ok) + def_cl = ALL_REGS; + else + def_cl = op_alt[def_match].cl; + check_and_make_def_conflict (alt, def, def_cl); + } + } } /* Mark early clobber hard registers of the current INSN as live (if Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c 2014-05-31 08:54:12.236234739 +0100 +++ gcc/config/i386/i386.c 2014-05-31 09:07:21.682714985 +0100 @@ -5874,8 +5874,11 @@ ix86_legitimate_combined_insn (rtx insn) /* Operand has no constraints, anything is OK. */ win = !n_alternatives; + alternative_mask enabled = recog_data.enabled_alternatives; for (j = 0; j < n_alternatives; j++, op_alt += n_operands) { + if (!TEST_BIT (enabled, j)) + continue; if (op_alt[i].anything_ok || (op_alt[i].matches != -1 && operands_match_p ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute 2014-05-31 9:15 ` [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute Richard Sandiford @ 2014-06-03 21:58 ` Jeff Law 2014-06-04 17:37 ` Richard Sandiford 0 siblings, 1 reply; 20+ messages in thread From: Jeff Law @ 2014-06-03 21:58 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/31/14 03:15, Richard Sandiford wrote: > As described in the covering note, it seems better to put the onus of > checking the enabled attribute on the passes that are walking each > alternative, like LRA does for its internal subpasses. That leads > to a nicer interface in patch 4 and would make it easier to precompute > the information at build time. (The only thing preventing that now is > the subunion class.) > > Thanks, > Richard > > > gcc/ > * recog.c (preprocess_constraints): Don't skip disabled alternatives. > * ira-lives.c (check_and_make_def_conflict): Check for disabled > alternatives. > (make_early_clobber_and_input_conflicts): Likewise. > * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. Did you spot check the other ports which utilized the enabled attribute to see if they need tweaking too? I see aarch64, alpha, arc, arm, avr, c6x m68k, mips, mn10300, nds32, s390, sh & sparc. I didn't check to see if any of them walk the alternatives in the backend. Assuming you at least spot checked them, then this patch is OK. jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute 2014-06-03 21:58 ` Jeff Law @ 2014-06-04 17:37 ` Richard Sandiford 0 siblings, 0 replies; 20+ messages in thread From: Richard Sandiford @ 2014-06-04 17:37 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Thanks for the reviews, now committed Jeff Law <law@redhat.com> writes: > On 05/31/14 03:15, Richard Sandiford wrote: >> As described in the covering note, it seems better to put the onus of >> checking the enabled attribute on the passes that are walking each >> alternative, like LRA does for its internal subpasses. That leads >> to a nicer interface in patch 4 and would make it easier to precompute >> the information at build time. (The only thing preventing that now is >> the subunion class.) >> >> Thanks, >> Richard >> >> >> gcc/ >> * recog.c (preprocess_constraints): Don't skip disabled alternatives. >> * ira-lives.c (check_and_make_def_conflict): Check for disabled >> alternatives. >> (make_early_clobber_and_input_conflicts): Likewise. >> * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. > Did you spot check the other ports which utilized the enabled attribute > to see if they need tweaking too? > > I see aarch64, alpha, arc, arm, avr, c6x m68k, mips, mn10300, nds32, > s390, sh & sparc. I didn't check to see if any of them walk the > alternatives in the backend. Yeah, the only port besides i386 to use preprocess_constraints is arm, but it only looks at alternative which_alternative. Richard ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/5] Cache recog_op_alt by insn code: main patch 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford ` (2 preceding siblings ...) 2014-05-31 9:15 ` [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute Richard Sandiford @ 2014-05-31 9:17 ` Richard Sandiford 2014-06-03 22:01 ` Jeff Law 2014-05-31 9:23 ` [PATCH 5/5] Reuse recog_op_alt cache in LRA Richard Sandiford 2014-06-03 21:38 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Jeff Law 5 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-31 9:17 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches This is the refreshed main patch. I've rearranged the preprocess_constraints code into three functions so that LRA can use it in patch 5. Thanks, Richard gcc/ * recog.h (operand_alternative): Convert reg_class, reject, matched and matches into bitfields. (preprocess_constraints): New overload. (preprocess_insn_constraints): New function. (preprocess_constraints): Take the insn as parameter. (recog_op_alt): Change into a pointer. (target_recog): Add x_op_alt. * recog.c (asm_op_alt): New variable. (recog_op_alt): Change into a pointer. (preprocess_constraints): New overload, replacing the old function definition with one that doesn't use global state. (preprocess_insn_constraints): New function. (preprocess_constraints): Use them. Take the insn as parameter. Use asm_op_alt for asms. (recog_init): Free existing x_op_alt entries. * ira-lives.c (check_and_make_def_conflict): Make operand_alternative pointer const. (make_early_clobber_and_input_conflicts): Likewise. (process_bb_node_lives): Pass the insn to process_constraints. * reg-stack.c (check_asm_stack_operands): Likewise. (subst_asm_stack_regs): Likewise. * regcprop.c (copyprop_hardreg_forward_1): Likewise. * regrename.c (build_def_use): Likewise. * sched-deps.c (sched_analyze_insn): Likewise. * sel-sched.c (get_reg_class, implicit_clobber_conflict_p): Likewise. * config/arm/arm.c (xscale_sched_adjust_cost): Likewise. (note_invalid_constants): Likewise. * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. (ix86_legitimate_combined_insn): Make operand_alternative pointer const. Index: gcc/recog.h =================================================================== --- gcc/recog.h 2014-05-31 09:02:35.956369716 +0100 +++ gcc/recog.h 2014-05-31 09:10:16.596150618 +0100 @@ -46,18 +46,18 @@ struct operand_alternative const char *constraint; /* The register class valid for this alternative (possibly NO_REGS). */ - enum reg_class cl; + ENUM_BITFIELD (reg_class) cl : 16; /* "Badness" of this alternative, computed from number of '?' and '!' characters in the constraint string. */ - unsigned int reject; + unsigned int reject : 16; /* -1 if no matching constraint was found, or an operand number. */ - int matches; + int matches : 8; /* The same information, but reversed: -1 if this operand is not matched by any other, or the operand number of the operand that matches this one. */ - int matched; + int matched : 8; /* Nonzero if '&' was found in the constraint string. */ unsigned int earlyclobber:1; @@ -77,6 +77,8 @@ struct operand_alternative /* Nonzero if 'X' was found in the constraint string, or if the constraint string for this alternative was empty. */ unsigned int anything_ok:1; + + unsigned int unused : 8; }; /* Return the class for operand I of alternative ALT, taking matching @@ -142,7 +144,10 @@ extern void insn_extract (rtx); extern void extract_insn (rtx); extern void extract_constrain_insn_cached (rtx); extern void extract_insn_cached (rtx); -extern void preprocess_constraints (void); +extern void preprocess_constraints (int, int, const char **, + operand_alternative *); +extern const operand_alternative *preprocess_insn_constraints (int); +extern void preprocess_constraints (rtx); extern rtx peep2_next_insn (int); extern int peep2_regno_dead_p (int, int); extern int peep2_reg_dead_p (int, rtx); @@ -264,8 +269,7 @@ struct recog_data_d extern struct recog_data_d recog_data; -extern struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS - * MAX_RECOG_ALTERNATIVES]; +extern const operand_alternative *recog_op_alt; /* Return a pointer to an array in which index OP describes the constraints on operand OP of the current instruction alternative (which_alternative). @@ -396,6 +400,7 @@ struct insn_data_d struct target_recog { bool x_initialized; alternative_mask x_enabled_alternatives[LAST_INSN_CODE]; + operand_alternative *x_op_alt[LAST_INSN_CODE]; }; extern struct target_recog default_target_recog; Index: gcc/recog.c =================================================================== --- gcc/recog.c 2014-05-31 09:07:21.669714878 +0100 +++ gcc/recog.c 2014-05-31 09:15:38.746800685 +0100 @@ -81,8 +81,11 @@ struct recog_data_d recog_data; /* Contains a vector of operand_alternative structures, such that operand OP of alternative A is at index A * n_operands + OP. Set up by preprocess_constraints. */ -struct operand_alternative recog_op_alt[MAX_RECOG_OPERANDS - * MAX_RECOG_ALTERNATIVES]; +const operand_alternative *recog_op_alt; + +/* Used to provide recog_op_alt for asms. */ +static operand_alternative asm_op_alt[MAX_RECOG_OPERANDS + * MAX_RECOG_ALTERNATIVES]; /* On return from `constrain_operands', indicate which alternative was satisfied. */ @@ -2324,26 +2327,23 @@ extract_insn (rtx insn) which_alternative = -1; } -/* After calling extract_insn, you can use this function to extract some - information from the constraint strings into a more usable form. - The collected data is stored in recog_op_alt. */ +/* Fill in OP_ALT_BASE for an instruction that has N_OPERANDS operands, + N_ALTERNATIVES alternatives and constraint strings CONSTRAINTS. + OP_ALT_BASE has N_ALTERNATIVES * N_OPERANDS entries and CONSTRAINTS + has N_OPERANDS entries. */ + void -preprocess_constraints (void) +preprocess_constraints (int n_operands, int n_alternatives, + const char **constraints, + operand_alternative *op_alt_base) { - int i; - - int n_operands = recog_data.n_operands; - int n_alternatives = recog_data.n_alternatives; - int n_entries = n_operands * n_alternatives; - memset (recog_op_alt, 0, n_entries * sizeof (struct operand_alternative)); - - for (i = 0; i < n_operands; i++) + for (int i = 0; i < n_operands; i++) { int j; struct operand_alternative *op_alt; - const char *p = recog_data.constraints[i]; + const char *p = constraints[i]; - op_alt = recog_op_alt; + op_alt = op_alt_base; for (j = 0; j < n_alternatives; j++, op_alt += n_operands) { @@ -2462,6 +2462,59 @@ preprocess_constraints (void) } } +/* Return an array of operand_alternative instructions for + instruction ICODE. */ + +const operand_alternative * +preprocess_insn_constraints (int icode) +{ + gcc_checking_assert (IN_RANGE (icode, 0, LAST_INSN_CODE)); + if (this_target_recog->x_op_alt[icode]) + return this_target_recog->x_op_alt[icode]; + + int n_operands = insn_data[icode].n_operands; + if (n_operands == 0) + return 0; + /* Always provide at least one alternative so that which_op_alt () + works correctly. If the instruction has 0 alternatives (i.e. all + constraint strings are empty) then each operand in this alternative + will have anything_ok set. */ + int n_alternatives = MAX (insn_data[icode].n_alternatives, 1); + int n_entries = n_operands * n_alternatives; + + operand_alternative *op_alt = XCNEWVEC (operand_alternative, n_entries); + const char **constraints = XALLOCAVEC (const char *, n_operands); + + for (int i = 0; i < n_operands; ++i) + constraints[i] = insn_data[icode].operand[i].constraint; + preprocess_constraints (n_operands, n_alternatives, constraints, op_alt); + + this_target_recog->x_op_alt[icode] = op_alt; + return op_alt; +} + +/* After calling extract_insn, you can use this function to extract some + information from the constraint strings into a more usable form. + The collected data is stored in recog_op_alt. */ + +void +preprocess_constraints (rtx insn) +{ + int icode = INSN_CODE (insn); + if (icode >= 0) + recog_op_alt = preprocess_insn_constraints (icode); + else + { + int n_operands = recog_data.n_operands; + int n_alternatives = recog_data.n_alternatives; + int n_entries = n_operands * n_alternatives; + memset (asm_op_alt, 0, n_entries * sizeof (operand_alternative)); + preprocess_constraints (n_operands, n_alternatives, + recog_data.constraints, asm_op_alt); + recog_op_alt = asm_op_alt; + } +} + /* Check the operands of an insn against the insn's operand constraints and return 1 if they are valid. The information about the insn's operands, constraints, operand modes @@ -4211,4 +4264,10 @@ recog_init () } memset (this_target_recog->x_enabled_alternatives, 0, sizeof (this_target_recog->x_enabled_alternatives)); + for (int i = 0; i < LAST_INSN_CODE; ++i) + if (this_target_recog->x_op_alt[i]) + { + free (this_target_recog->x_op_alt[i]); + this_target_recog->x_op_alt[i] = 0; + } } Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2014-05-31 09:07:21.670714886 +0100 +++ gcc/ira-lives.c 2014-05-31 09:10:16.598150634 +0100 @@ -625,7 +625,7 @@ check_and_make_def_conflict (int alt, in advance_p = true; int n_operands = recog_data.n_operands; - operand_alternative *op_alt = &recog_op_alt[alt * n_operands]; + const operand_alternative *op_alt = &recog_op_alt[alt * n_operands]; for (use = 0; use < n_operands; use++) { int alt1; @@ -646,7 +646,8 @@ check_and_make_def_conflict (int alt, in { if (!TEST_BIT (enabled, alt1)) continue; - operand_alternative *op_alt1 = &recog_op_alt[alt1 * n_operands]; + const operand_alternative *op_alt1 + = &recog_op_alt[alt1 * n_operands]; if (op_alt1[use].matches == def || (use < n_operands - 1 && recog_data.constraints[use][0] == '%' @@ -692,7 +693,7 @@ make_early_clobber_and_input_conflicts ( int n_alternatives = recog_data.n_alternatives; int n_operands = recog_data.n_operands; alternative_mask enabled = recog_data.enabled_alternatives; - operand_alternative *op_alt = recog_op_alt; + const operand_alternative *op_alt = recog_op_alt; for (alt = 0; alt < n_alternatives; alt++, op_alt += n_operands) if (TEST_BIT (enabled, alt)) for (def = 0; def < n_operands; def++) @@ -1251,7 +1252,7 @@ process_bb_node_lives (ira_loop_tree_nod } extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); process_single_reg_class_operands (false, freq); /* See which defined values die here. */ Index: gcc/reg-stack.c =================================================================== --- gcc/reg-stack.c 2014-05-31 09:02:35.968369814 +0100 +++ gcc/reg-stack.c 2014-05-31 09:10:16.601150659 +0100 @@ -471,7 +471,7 @@ check_asm_stack_operands (rtx insn) extract_insn (insn); constrain_operands (1); - preprocess_constraints (); + preprocess_constraints (insn); get_asm_operands_in_out (body, &n_outputs, &n_inputs); @@ -2029,7 +2029,7 @@ subst_asm_stack_regs (rtx insn, stack_pt extract_insn (insn); constrain_operands (1); - preprocess_constraints (); + preprocess_constraints (insn); const operand_alternative *op_alt = which_op_alt (); get_asm_operands_in_out (body, &n_outputs, &n_inputs); Index: gcc/regcprop.c =================================================================== --- gcc/regcprop.c 2014-05-31 09:02:35.957369724 +0100 +++ gcc/regcprop.c 2014-05-31 09:10:16.601150659 +0100 @@ -774,7 +774,7 @@ copyprop_hardreg_forward_1 (basic_block extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); const operand_alternative *op_alt = which_op_alt (); n_ops = recog_data.n_operands; is_asm = asm_noperands (PATTERN (insn)) >= 0; @@ -877,7 +877,7 @@ copyprop_hardreg_forward_1 (basic_block extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); } /* Otherwise, try all valid registers and see if its valid. */ @@ -905,7 +905,7 @@ copyprop_hardreg_forward_1 (basic_block extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); } } } Index: gcc/regrename.c =================================================================== --- gcc/regrename.c 2014-05-31 09:02:35.958369732 +0100 +++ gcc/regrename.c 2014-05-31 09:10:16.602150667 +0100 @@ -1570,7 +1570,7 @@ build_def_use (basic_block bb) extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); const operand_alternative *op_alt = which_op_alt (); n_ops = recog_data.n_operands; untracked_operands = 0; Index: gcc/sched-deps.c =================================================================== --- gcc/sched-deps.c 2014-05-31 08:54:09.086208879 +0100 +++ gcc/sched-deps.c 2014-05-31 09:10:16.615150774 +0100 @@ -2865,7 +2865,7 @@ sched_analyze_insn (struct deps_desc *de HARD_REG_SET temp; extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); ira_implicitly_set_insn_hard_regs (&temp); AND_COMPL_HARD_REG_SET (temp, ira_no_alloc_regs); IOR_HARD_REG_SET (implicit_reg_pending_clobbers, temp); Index: gcc/sel-sched.c =================================================================== --- gcc/sel-sched.c 2014-05-31 09:02:35.970369830 +0100 +++ gcc/sel-sched.c 2014-05-31 09:10:16.617150790 +0100 @@ -1019,7 +1019,7 @@ get_reg_class (rtx insn) extract_insn (insn); if (! constrain_operands (1)) fatal_insn_not_found (insn); - preprocess_constraints (); + preprocess_constraints (insn); n_ops = recog_data.n_operands; const operand_alternative *op_alt = which_op_alt (); @@ -2134,7 +2134,7 @@ implicit_clobber_conflict_p (insn_t thro /* Calculate implicit clobbers. */ extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); ira_implicitly_set_insn_hard_regs (&temp); AND_COMPL_HARD_REG_SET (temp, ira_no_alloc_regs); Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c 2014-05-31 09:02:35.966369798 +0100 +++ gcc/config/arm/arm.c 2014-05-31 09:10:16.626150864 +0100 @@ -11335,7 +11335,7 @@ xscale_sched_adjust_cost (rtx insn, rtx that overlaps with SHIFTED_OPERAND, then we have increase the cost of this dependency. */ extract_insn (dep); - preprocess_constraints (); + preprocess_constraints (dep); for (opno = 0; opno < recog_data.n_operands; opno++) { /* We can ignore strict inputs. */ @@ -16870,7 +16870,7 @@ note_invalid_constants (rtx insn, HOST_W /* Fill in recog_op_alt with information about the constraints of this insn. */ - preprocess_constraints (); + preprocess_constraints (insn); const operand_alternative *op_alt = which_op_alt (); for (opno = 0; opno < recog_data.n_operands; opno++) Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c 2014-05-31 09:07:21.682714985 +0100 +++ gcc/config/i386/i386.c 2014-05-31 09:10:16.638150962 +0100 @@ -5826,7 +5826,7 @@ ix86_legitimate_combined_insn (rtx insn) int i; extract_insn (insn); - preprocess_constraints (); + preprocess_constraints (insn); int n_operands = recog_data.n_operands; int n_alternatives = recog_data.n_alternatives; @@ -5834,7 +5834,7 @@ ix86_legitimate_combined_insn (rtx insn) { rtx op = recog_data.operand[i]; enum machine_mode mode = GET_MODE (op); - operand_alternative *op_alt; + const operand_alternative *op_alt; int offset = 0; bool win; int j; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] Cache recog_op_alt by insn code: main patch 2014-05-31 9:17 ` [PATCH 4/5] Cache recog_op_alt by insn code: main patch Richard Sandiford @ 2014-06-03 22:01 ` Jeff Law 0 siblings, 0 replies; 20+ messages in thread From: Jeff Law @ 2014-06-03 22:01 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/31/14 03:17, Richard Sandiford wrote: > This is the refreshed main patch. I've rearranged the preprocess_constraints > code into three functions so that LRA can use it in patch 5. > > Thanks, > Richard > > > gcc/ > * recog.h (operand_alternative): Convert reg_class, reject, > matched and matches into bitfields. > (preprocess_constraints): New overload. > (preprocess_insn_constraints): New function. > (preprocess_constraints): Take the insn as parameter. > (recog_op_alt): Change into a pointer. > (target_recog): Add x_op_alt. > * recog.c (asm_op_alt): New variable. > (recog_op_alt): Change into a pointer. > (preprocess_constraints): New overload, replacing the old function > definition with one that doesn't use global state. > (preprocess_insn_constraints): New function. > (preprocess_constraints): Use them. Take the insn as parameter. > Use asm_op_alt for asms. > (recog_init): Free existing x_op_alt entries. > * ira-lives.c (check_and_make_def_conflict): Make operand_alternative > pointer const. > (make_early_clobber_and_input_conflicts): Likewise. > (process_bb_node_lives): Pass the insn to process_constraints. > * reg-stack.c (check_asm_stack_operands): Likewise. > (subst_asm_stack_regs): Likewise. > * regcprop.c (copyprop_hardreg_forward_1): Likewise. > * regrename.c (build_def_use): Likewise. > * sched-deps.c (sched_analyze_insn): Likewise. > * sel-sched.c (get_reg_class, implicit_clobber_conflict_p): Likewise. > * config/arm/arm.c (xscale_sched_adjust_cost): Likewise. > (note_invalid_constants): Likewise. > * config/i386/i386.c (ix86_legitimate_combined_insn): Likewise. > (ix86_legitimate_combined_insn): Make operand_alternative pointer > const. OK. jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/5] Reuse recog_op_alt cache in LRA 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford ` (3 preceding siblings ...) 2014-05-31 9:17 ` [PATCH 4/5] Cache recog_op_alt by insn code: main patch Richard Sandiford @ 2014-05-31 9:23 ` Richard Sandiford 2014-06-03 22:02 ` Jeff Law 2014-06-03 21:38 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Jeff Law 5 siblings, 1 reply; 20+ messages in thread From: Richard Sandiford @ 2014-05-31 9:23 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches The operand_alternative cache was LRA's only per-subtarget state so a lot of this patch is removing the associated initialisation and target_globals stuff. I called the preprocess_constraints functions in lra_set_insn_recog_data rather than setup_operand_alternative because lra_set_insn_recog_data already has a local constraints array (needed for asms). setup_operand_alternative is now only called in the uncached case. Thanks, Richard gcc/ * lra-int.h (lra_static_insn_data): Make operand_alternative a const pointer. (target_lra_int, default_target_lra_int, this_target_lra_int) (op_alt_data): Delete. * lra.h (lra_init): Delete. * lra.c (default_target_lra_int, this_target_lra_int): Delete. (init_insn_code_data_once): Remove op_alt_data handling. (finish_insn_code_data_once): Likewise. (init_op_alt_data): Delete. (get_static_insn_data): Initialize operand_alternative to null. (free_insn_recog_data): Cast operand_alternative before freeing it. (setup_operand_alternative): Take the operand_alternative as parameter and assume it isn't already cached in the static insn data. (lra_set_insn_recog_data): Update accordingly. (lra_init): Delete. * ira.c (ira_init): Don't call lra_init. * target-globals.h (this_target_lra_int): Declare. (target_globals): Remove lra_int. (restore_target_globals): Update accordingly. * target-globals.c: Don't include lra-int.h. (default_target_globals, save_target_globals): Remove lra_int. Index: gcc/lra-int.h =================================================================== --- gcc/lra-int.h 2014-05-31 08:54:08.894207303 +0100 +++ gcc/lra-int.h 2014-05-31 09:25:56.876885502 +0100 @@ -198,7 +198,7 @@ struct lra_static_insn_data /* Array [n_alternatives][n_operand] of static constraint info for given operand in given alternative. This info can be changed if the target reg info is changed. */ - struct operand_alternative *operand_alternative; + const struct operand_alternative *operand_alternative; }; /* LRA internal info about an insn (LRA internal insn @@ -495,21 +495,3 @@ lra_assign_reg_val (int from, int to) lra_reg_info[to].val = lra_reg_info[from].val; lra_reg_info[to].offset = lra_reg_info[from].offset; } -\f - -struct target_lra_int -{ - /* Map INSN_UID -> the operand alternative data (NULL if unknown). - We assume that this data is valid until register info is changed - because classes in the data can be changed. */ - struct operand_alternative *x_op_alt_data[LAST_INSN_CODE]; -}; - -extern struct target_lra_int default_target_lra_int; -#if SWITCHABLE_TARGET -extern struct target_lra_int *this_target_lra_int; -#else -#define this_target_lra_int (&default_target_lra_int) -#endif - -#define op_alt_data (this_target_lra_int->x_op_alt_data) Index: gcc/lra.h =================================================================== --- gcc/lra.h 2014-05-31 08:54:08.894207303 +0100 +++ gcc/lra.h 2014-05-31 09:25:56.951886120 +0100 @@ -36,5 +36,4 @@ extern rtx lra_create_new_reg (enum mach extern rtx lra_eliminate_regs (rtx, enum machine_mode, rtx); extern void lra (FILE *); extern void lra_init_once (void); -extern void lra_init (void); extern void lra_finish_once (void); Index: gcc/lra.c =================================================================== --- gcc/lra.c 2014-05-31 08:54:08.894207303 +0100 +++ gcc/lra.c 2014-05-31 09:25:56.891885625 +0100 @@ -553,11 +553,6 @@ finish_insn_regs (void) /* This page contains code dealing LRA insn info (or in other words LRA internal insn representation). */ -struct target_lra_int default_target_lra_int; -#if SWITCHABLE_TARGET -struct target_lra_int *this_target_lra_int = &default_target_lra_int; -#endif - /* Map INSN_CODE -> the static insn data. This info is valid during all translation unit. */ struct lra_static_insn_data *insn_code_data[LAST_INSN_CODE]; @@ -599,7 +594,6 @@ static struct lra_static_insn_data debug init_insn_code_data_once (void) { memset (insn_code_data, 0, sizeof (insn_code_data)); - memset (op_alt_data, 0, sizeof (op_alt_data)); } /* Called once per compiler work to finalize some LRA data related to @@ -613,25 +607,9 @@ finish_insn_code_data_once (void) { if (insn_code_data[i] != NULL) free (insn_code_data[i]); - if (op_alt_data[i] != NULL) - free (op_alt_data[i]); } } -/* Initialize LRA info about operands in insn alternatives. */ -static void -init_op_alt_data (void) -{ - int i; - - for (i = 0; i < LAST_INSN_CODE; i++) - if (op_alt_data[i] != NULL) - { - free (op_alt_data[i]); - op_alt_data[i] = NULL; - } -} - /* Return static insn data, allocate and setup if necessary. Although dup_num is static data (it depends only on icode), to set it up we need to extract insn first. So recog_data should be valid for @@ -650,6 +628,7 @@ get_static_insn_data (int icode, int nop + sizeof (struct lra_operand_data) * nop + sizeof (int) * ndup; data = XNEWVAR (struct lra_static_insn_data, n_bytes); + data->operand_alternative = NULL; data->n_operands = nop; data->n_dups = ndup; data->n_alternatives = nalt; @@ -727,7 +706,8 @@ free_insn_recog_data (lra_insn_recog_dat if (data->icode < 0 && NONDEBUG_INSN_P (data->insn)) { if (data->insn_static_data->operand_alternative != NULL) - free (data->insn_static_data->operand_alternative); + free (const_cast <operand_alternative *> + (data->insn_static_data->operand_alternative)); free_insn_regs (data->insn_static_data->hard_regs); free (data->insn_static_data); } @@ -752,173 +732,38 @@ finish_insn_recog_data (void) /* Setup info about operands in alternatives of LRA DATA of insn. */ static void -setup_operand_alternative (lra_insn_recog_data_t data) +setup_operand_alternative (lra_insn_recog_data_t data, + const operand_alternative *op_alt) { - int i, nop, nalt; + int i, j, nop, nalt; int icode = data->icode; struct lra_static_insn_data *static_data = data->insn_static_data; - if (icode >= 0 - && (static_data->operand_alternative = op_alt_data[icode]) != NULL) - return; static_data->commutative = -1; nop = static_data->n_operands; - if (nop == 0) - { - static_data->operand_alternative = NULL; - return; - } nalt = static_data->n_alternatives; - static_data->operand_alternative = XNEWVEC (struct operand_alternative, - nalt * nop); - memset (static_data->operand_alternative, 0, - nalt * nop * sizeof (struct operand_alternative)); - if (icode >= 0) - op_alt_data[icode] = static_data->operand_alternative; + static_data->operand_alternative = op_alt; for (i = 0; i < nop; i++) { - int j; - struct operand_alternative *op_alt_start, *op_alt; - const char *p = static_data->operand[i].constraint; - - static_data->operand[i].early_clobber = 0; - op_alt_start = &static_data->operand_alternative[i]; - - for (j = 0; j < nalt; j++) - { - op_alt = op_alt_start + j * nop; - op_alt->cl = NO_REGS; - op_alt->constraint = p; - op_alt->matches = -1; - op_alt->matched = -1; - - if (*p == '\0' || *p == ',') - { - op_alt->anything_ok = 1; - continue; - } - - for (;;) - { - char c = *p; - if (c == '#') - do - c = *++p; - while (c != ',' && c != '\0'); - if (c == ',' || c == '\0') - { - p++; - break; - } - - switch (c) - { - case '=': case '+': case '*': - case 'E': case 'F': case 'G': case 'H': - case 's': case 'i': case 'n': - case 'I': case 'J': case 'K': case 'L': - case 'M': case 'N': case 'O': case 'P': - /* These don't say anything we care about. */ - break; - - case '%': - /* We currently only support one commutative pair of - operands. */ - if (static_data->commutative < 0) - static_data->commutative = i; - else - lra_assert (data->icode < 0); /* Asm */ - - /* The last operand should not be marked - commutative. */ - lra_assert (i != nop - 1); - break; - - case '?': - op_alt->reject += LRA_LOSER_COST_FACTOR; - break; - case '!': - op_alt->reject += LRA_MAX_REJECT; - break; - case '&': - op_alt->earlyclobber = 1; - static_data->operand[i].early_clobber = 1; - break; - - case '0': case '1': case '2': case '3': case '4': - case '5': case '6': case '7': case '8': case '9': - { - char *end; - op_alt->matches = strtoul (p, &end, 10); - static_data->operand_alternative - [j * nop + op_alt->matches].matched = i; - p = end; - } - continue; - - case TARGET_MEM_CONSTRAINT: - op_alt->memory_ok = 1; - break; - case '<': - op_alt->decmem_ok = 1; - break; - case '>': - op_alt->incmem_ok = 1; - break; - case 'V': - op_alt->nonoffmem_ok = 1; - break; - case 'o': - op_alt->offmem_ok = 1; - break; - case 'X': - op_alt->anything_ok = 1; - break; - - case 'p': - static_data->operand[i].is_address = true; - op_alt->is_address = 1; - op_alt->cl = (reg_class_subunion[(int) op_alt->cl] - [(int) base_reg_class (VOIDmode, - ADDR_SPACE_GENERIC, - ADDRESS, SCRATCH)]); - break; - - case 'g': - case 'r': - op_alt->cl = - reg_class_subunion[(int) op_alt->cl][(int) GENERAL_REGS]; - break; - - default: - if (EXTRA_MEMORY_CONSTRAINT (c, p)) - { - op_alt->memory_ok = 1; - break; - } - if (EXTRA_ADDRESS_CONSTRAINT (c, p)) - { - static_data->operand[i].is_address = true; - op_alt->is_address = 1; - op_alt->cl - = (reg_class_subunion - [(int) op_alt->cl] - [(int) base_reg_class (VOIDmode, ADDR_SPACE_GENERIC, - ADDRESS, SCRATCH)]); - break; - } - - op_alt->cl - = (reg_class_subunion - [(int) op_alt->cl] - [(int) - REG_CLASS_FROM_CONSTRAINT ((unsigned char) c, p)]); - break; - } - p += CONSTRAINT_LEN (c, p); - } + static_data->operand[i].early_clobber = false; + static_data->operand[i].is_address = false; + if (static_data->operand[i].constraint[0] == '%') + { + /* We currently only support one commutative pair of operands. */ + if (static_data->commutative < 0) + static_data->commutative = i; + else + lra_assert (icode < 0); /* Asm */ + /* The last operand should not be marked commutative. */ + lra_assert (i != nop - 1); } } + for (j = 0; j < nalt; j++) + for (i = 0; i < nop; i++, op_alt++) + { + static_data->operand[i].early_clobber |= op_alt->earlyclobber; + static_data->operand[i].is_address |= op_alt->is_address; + } } /* Recursively process X and collect info about registers, which are @@ -1077,12 +922,13 @@ lra_set_insn_recog_data (rtx insn) } if (icode < 0) { - int nop; + int nop, nalt; enum machine_mode operand_mode[MAX_RECOG_OPERANDS]; const char *constraints[MAX_RECOG_OPERANDS]; nop = asm_noperands (PATTERN (insn)); data->operand_loc = data->dup_loc = NULL; + nalt = 1; if (nop < 0) { /* Its is a special insn like USE or CLOBBER. We should @@ -1092,7 +938,7 @@ lra_set_insn_recog_data (rtx insn) || GET_CODE (PATTERN (insn)) == CLOBBER || GET_CODE (PATTERN (insn)) == ASM_INPUT); data->insn_static_data = insn_static_data - = get_static_insn_data (-1, 0, 0, 1); + = get_static_insn_data (-1, 0, 0, nalt); } else { @@ -1106,16 +952,15 @@ lra_set_insn_recog_data (rtx insn) decode_asm_operands (PATTERN (insn), NULL, data->operand_loc, constraints, operand_mode, NULL); - n = 1; if (nop > 0) { const char *p = recog_data.constraints[0]; for (p = constraints[0]; *p; p++) - n += *p == ','; + nalt += *p == ','; } data->insn_static_data = insn_static_data - = get_static_insn_data (-1, nop, 0, n); + = get_static_insn_data (-1, nop, 0, nalt); for (i = 0; i < nop; i++) { insn_static_data->operand[i].mode = operand_mode[i]; @@ -1131,6 +976,13 @@ lra_set_insn_recog_data (rtx insn) : insn_static_data->operand[i].constraint[0] == '+' ? OP_INOUT : OP_IN); data->enabled_alternatives = ALL_ALTERNATIVES; + if (nop > 0) + { + operand_alternative *op_alt = XCNEWVEC (operand_alternative, + nalt * nop); + preprocess_constraints (nop, nalt, constraints, op_alt); + setup_operand_alternative (data, op_alt); + } } else { @@ -1158,6 +1010,11 @@ lra_set_insn_recog_data (rtx insn) } data->dup_loc = locs; data->enabled_alternatives = get_enabled_alternatives (insn); + const operand_alternative *op_alt = preprocess_insn_constraints (icode); + if (!insn_static_data->operand_alternative) + setup_operand_alternative (data, op_alt); + else if (op_alt != insn_static_data->operand_alternative) + insn_static_data->operand_alternative = op_alt; } if (GET_CODE (PATTERN (insn)) == CLOBBER || GET_CODE (PATTERN (insn)) == USE) insn_static_data->hard_regs = NULL; @@ -1165,7 +1022,6 @@ lra_set_insn_recog_data (rtx insn) insn_static_data->hard_regs = collect_non_operand_hard_regs (&PATTERN (insn), data, NULL, OP_IN, false); - setup_operand_alternative (data); data->arg_hard_regs = NULL; if (CALL_P (insn)) { @@ -2451,13 +2307,6 @@ lra_init_once (void) init_insn_code_data_once (); } -/* Initialize LRA whenever register-related information is changed. */ -void -lra_init (void) -{ - init_op_alt_data (); -} - /* Called once per compiler to finish LRA data which are initialize once. */ void Index: gcc/ira.c =================================================================== --- gcc/ira.c 2014-05-31 08:54:08.894207303 +0100 +++ gcc/ira.c 2014-05-31 09:25:56.934885982 +0100 @@ -1715,7 +1715,6 @@ ira_init (void) clarify_prohibited_class_mode_regs (); setup_hard_regno_aclass (); ira_init_costs (); - lra_init (); } /* Function called once at the end of compiler work. */ Index: gcc/target-globals.h =================================================================== --- gcc/target-globals.h 2014-05-31 08:54:08.894207303 +0100 +++ gcc/target-globals.h 2014-05-31 09:25:56.914885815 +0100 @@ -33,7 +33,6 @@ #define TARGET_GLOBALS_H 1 extern struct target_cfgloop *this_target_cfgloop; extern struct target_ira *this_target_ira; extern struct target_ira_int *this_target_ira_int; -extern struct target_lra_int *this_target_lra_int; extern struct target_builtins *this_target_builtins; extern struct target_gcse *this_target_gcse; extern struct target_bb_reorder *this_target_bb_reorder; @@ -53,7 +52,6 @@ struct GTY(()) target_globals { struct target_cfgloop *GTY((skip)) cfgloop; void *GTY((atomic)) ira; void *GTY((atomic)) ira_int; - void *GTY((atomic)) lra_int; struct target_builtins *GTY((skip)) builtins; struct target_gcse *GTY((skip)) gcse; struct target_bb_reorder *GTY((skip)) bb_reorder; @@ -81,7 +79,6 @@ restore_target_globals (struct target_gl this_target_cfgloop = g->cfgloop; this_target_ira = (struct target_ira *) g->ira; this_target_ira_int = (struct target_ira_int *) g->ira_int; - this_target_lra_int = (struct target_lra_int *) g->lra_int; this_target_builtins = g->builtins; this_target_gcse = g->gcse; this_target_bb_reorder = g->bb_reorder; Index: gcc/target-globals.c =================================================================== --- gcc/target-globals.c 2014-05-31 08:54:08.894207303 +0100 +++ gcc/target-globals.c 2014-05-31 09:25:56.909885775 +0100 @@ -38,7 +38,6 @@ Software Foundation; either version 3, o #include "libfuncs.h" #include "cfgloop.h" #include "ira-int.h" -#include "lra-int.h" #include "builtins.h" #include "gcse.h" #include "bb-reorder.h" @@ -59,7 +58,6 @@ struct target_globals default_target_glo &default_target_cfgloop, &default_target_ira, &default_target_ira_int, - &default_target_lra_int, &default_target_builtins, &default_target_gcse, &default_target_bb_reorder, @@ -96,7 +94,6 @@ save_target_globals (void) g->cfgloop = &p->cfgloop; g->ira = ggc_internal_cleared_alloc (sizeof (struct target_ira)); g->ira_int = ggc_internal_cleared_alloc (sizeof (struct target_ira_int)); - g->lra_int = ggc_internal_cleared_alloc (sizeof (struct target_lra_int)); g->builtins = &p->builtins; g->gcse = &p->gcse; g->bb_reorder = &p->bb_reorder; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] Reuse recog_op_alt cache in LRA 2014-05-31 9:23 ` [PATCH 5/5] Reuse recog_op_alt cache in LRA Richard Sandiford @ 2014-06-03 22:02 ` Jeff Law 0 siblings, 0 replies; 20+ messages in thread From: Jeff Law @ 2014-06-03 22:02 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/31/14 03:22, Richard Sandiford wrote: > The operand_alternative cache was LRA's only per-subtarget state > so a lot of this patch is removing the associated initialisation > and target_globals stuff. > > I called the preprocess_constraints functions in lra_set_insn_recog_data > rather than setup_operand_alternative because lra_set_insn_recog_data > already has a local constraints array (needed for asms). > setup_operand_alternative is now only called in the uncached case. > > Thanks, > Richard > > > gcc/ > * lra-int.h (lra_static_insn_data): Make operand_alternative a > const pointer. > (target_lra_int, default_target_lra_int, this_target_lra_int) > (op_alt_data): Delete. > * lra.h (lra_init): Delete. > * lra.c (default_target_lra_int, this_target_lra_int): Delete. > (init_insn_code_data_once): Remove op_alt_data handling. > (finish_insn_code_data_once): Likewise. > (init_op_alt_data): Delete. > (get_static_insn_data): Initialize operand_alternative to null. > (free_insn_recog_data): Cast operand_alternative before freeing it. > (setup_operand_alternative): Take the operand_alternative as > parameter and assume it isn't already cached in the static > insn data. > (lra_set_insn_recog_data): Update accordingly. > (lra_init): Delete. > * ira.c (ira_init): Don't call lra_init. > * target-globals.h (this_target_lra_int): Declare. > (target_globals): Remove lra_int. > (restore_target_globals): Update accordingly. > * target-globals.c: Don't include lra-int.h. > (default_target_globals, save_target_globals): Remove lra_int. Also good for the trunk. Got to love patches that delete big blobs of code :-) jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] Cache recog_op_alt by insn code, take 2 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford ` (4 preceding siblings ...) 2014-05-31 9:23 ` [PATCH 5/5] Reuse recog_op_alt cache in LRA Richard Sandiford @ 2014-06-03 21:38 ` Jeff Law 2014-06-04 17:39 ` Richard Sandiford 5 siblings, 1 reply; 20+ messages in thread From: Jeff Law @ 2014-06-03 21:38 UTC (permalink / raw) To: gcc-patches, rdsandiford On 05/31/14 03:02, Richard Sandiford wrote: > Sorry Jeff, while working on the follow-on LRA patch I came across > a couple of problems, so I need another round on this. It happens, particularly for conceptual changes like this. I don't consider that a problem at all -- we're already in agreement on the overall direction this work is going, the rest is the implementation details. With your long history with GCC, you get a lot of trust and leeway. > First of all, I mentioned doing a follow-on patch to make LRA and > recog use the same cache for operand_alternatives. I hadn't realised > until I wrote that patch that there's a significant difference between > the LRA and recog_op_alt versions of the information: recog_op_alt > groups alternatives together by operand whereas LRA groups operands > together by alternative. So in the cached flat array, recog_op_alt > uses [op * n_alternatives + alt] whereas LRA uses [alt * n_operands + nop]. > The two could only share a cache if they used the same order. The kind of thing that becomes obvious once you try to make it work :-) > > I think the LRA ordering makes more sense. Quite a few places are > only interested in alternative which_alternative, so grouping operands > together by alternative gives a natural subarray. And there are places > in IRA (which uses recog_op_alt rather than the LRA information) where > the "for each alternative" loop is the outer one. Yea, I would think that generally we're going to be interested in major indexing by which_alternative rather than the operands. > > A second difference was that preprocess_constraints skips disabled > alternatives while LRA's setup_operand_alternative doesn't; LRA just > checks for disabled alternatives when walking the array instead. > That should make no difference in practice, but since the LRA approach > would also make it easier to precompute the array at build time, > I thought it would be a better way to go. It also gives a cleaner > interface: we can have a preprocess_insn_constraints function that > takes a (define_insn-based) insn and returns the operand_alternative > information without any global state. Sounds good. Do you think prebuilding those arrays once at build time is likely to have a measurable impact? I realize you probably haven't done measurements, just looking for your gut instinct here. > > Also, it turns out that sel-sched.c, regcprop.c and regrename.c > modify the recog_op_alt structure as a convenient way of handling > matching constraints. That obviously isn't a good idea if we want > other passes to reuse the data later. I've fixed that and made the > types "const" so that new instances shouldn't creep in. Ick. But glad to see the introduction of const to prevent this from happening again. > > Also, I hadn't realised that a define_insn with no constraints > at all has 0 alternatives rather than 1. Some passes nevertheless > unconditionally access the recog_op_alt information for alternative > which_alternative. > > I could have fixed that by making n_alternatives always be >= 1 > in the static insn table. Several places do have fast paths for > n_alternatives == 0 though, so I thought it would be better to > handle the 0 alternative case in preprocess_constraints as well. > All it needs is a MIN (..., 1). Funny thing is I suspect the n_alternatives == 0 case is rare in practice though. >>> - "matched" and "matches" are operand numbers and so fit in 8 bits >> OK. This could also be smaller, don't we have an upper limit of 32 (or >> 30?) on the number of operands appearing in an insn. It'd be a way to >> get a few more bits if we need them someday. > > Yeah, we could probably squeeze everything into a single int if we needed > to and if we were prepared to have non-byte-sized integer fields. > Going from 2 ints to 1 int wouldn't help LP64 hosts as things stand > though, since we have a pointer at the beginning of the struct. Right and non-LP64 hosts are becoming more and more rare. And the masking necessary may eat up any benefits we'd get, hard to know either way. > > Boostrapped & regression-tested on x86_64-linux-gnu. Also tested by > building gcc.dg, g++.dg and gcc.c-torture for one target per config/ > directory and making sure that there were no asm differences. Hoping to slog through them this afternoon. Time is a bit tight though. jeff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] Cache recog_op_alt by insn code, take 2 2014-06-03 21:38 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Jeff Law @ 2014-06-04 17:39 ` Richard Sandiford 0 siblings, 0 replies; 20+ messages in thread From: Richard Sandiford @ 2014-06-04 17:39 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Jeff Law <law@redhat.com> writes: > On 05/31/14 03:02, Richard Sandiford wrote: >> A second difference was that preprocess_constraints skips disabled >> alternatives while LRA's setup_operand_alternative doesn't; LRA just >> checks for disabled alternatives when walking the array instead. >> That should make no difference in practice, but since the LRA approach >> would also make it easier to precompute the array at build time, >> I thought it would be a better way to go. It also gives a cleaner >> interface: we can have a preprocess_insn_constraints function that >> takes a (define_insn-based) insn and returns the operand_alternative >> information without any global state. > Sounds good. Do you think prebuilding those arrays once at build time > is likely to have a measurable impact? I realize you probably haven't > done measurements, just looking for your gut instinct here. I have a local patch that does it, but unfortunately it doesn't seem to make a measurable difference. I'll try measuring it again once other lower-hanging fruit are out of the way. >> Also, I hadn't realised that a define_insn with no constraints >> at all has 0 alternatives rather than 1. Some passes nevertheless >> unconditionally access the recog_op_alt information for alternative >> which_alternative. >> >> I could have fixed that by making n_alternatives always be >= 1 >> in the static insn table. Several places do have fast paths for >> n_alternatives == 0 though, so I thought it would be better to >> handle the 0 alternative case in preprocess_constraints as well. >> All it needs is a MIN (..., 1). > Funny thing is I suspect the n_alternatives == 0 case is rare in > practice though. Yeah, probably. :-) Thanks, Richard ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-06-04 17:39 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-20 8:19 RFA: cache recog_op_alt by insn code Richard Sandiford 2014-05-20 18:04 ` Jeff Law 2014-05-26 19:21 ` Require '%' to be at the beginning of a constraint string Richard Sandiford 2014-05-27 17:41 ` Jeff Law 2014-05-28 19:50 ` Richard Sandiford 2014-05-30 16:24 ` Jeff Law 2014-05-31 9:02 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Richard Sandiford 2014-05-31 9:06 ` [PATCH 1/5] Flatten recog_op_alt and reorder entries Richard Sandiford 2014-06-03 21:50 ` Jeff Law 2014-05-31 9:09 ` [PATCH 2/5] Don't modify recog_op_alt after preprocess_constraints Richard Sandiford 2014-06-03 21:53 ` Jeff Law 2014-05-31 9:15 ` [PATCH 3/5] Make recog_op_alt consumers check the enabled attribute Richard Sandiford 2014-06-03 21:58 ` Jeff Law 2014-06-04 17:37 ` Richard Sandiford 2014-05-31 9:17 ` [PATCH 4/5] Cache recog_op_alt by insn code: main patch Richard Sandiford 2014-06-03 22:01 ` Jeff Law 2014-05-31 9:23 ` [PATCH 5/5] Reuse recog_op_alt cache in LRA Richard Sandiford 2014-06-03 22:02 ` Jeff Law 2014-06-03 21:38 ` [PATCH 0/5] Cache recog_op_alt by insn code, take 2 Jeff Law 2014-06-04 17:39 ` Richard Sandiford
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).