* RFA: cache enabled attribute by insn code
@ 2014-05-20 8:16 Richard Sandiford
2014-05-20 8:17 ` Richard Sandiford
2014-05-20 17:50 ` Jeff Law
0 siblings, 2 replies; 25+ messages in thread
From: Richard Sandiford @ 2014-05-20 8:16 UTC (permalink / raw)
To: gcc-patches
get_attr_enabled was showing up high in a -O0 compile of fold-const.ii.
At the moment, extract_insn calls this function for every alternative
on each extraction, which can be expensive for instructions like
moves that have many alternatives.
The attribute is only supposed to depend on the insn code and the
current target. It isn't allowed to depend on the values of operands.
LRA already makes use of this to cache the enabled attributes based on code.
This patch makes the caching compiler-wide. It also converts the bool
array into a plain int bitfield, since at the moment we don't allow more
than 30 alternatives. (It's possible we might want to increase it
beyond 32 at some point, but then we can change the type to uint64_t
instead. Wanting to go beyond 64 would suggest deeper problems
somewhere. :-))
The patch gives a consistent compile-time improvement of about ~3.5%
on the -O0 fold-const.ii testcase.
There were a few instances of the construct:
cl = NO_REGS;
for (ignore_p = false, curr_alt = 0;
(c = *constraints);
constraints += CONSTRAINT_LEN (c, constraints))
if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
ignore_p = true;
else if (c == ',')
{
curr_alt++;
ignore_p = false;
}
else if (! ignore_p)
This had the effect of ignoring all alternatives after the first
attribute-disabled one, since once we found a disabled alternative we'd
always enter the first arm of the "if" and never increment curr_alt.
I don't think that was intentional.
Tested on x86_64-linux-gnu. OK to install?
I notice ira_setup_alts is using a HARD_REG_SET to store a mask
of alternatives. If this patch is OK, I'll follow up with a patch
to use alternative_mask there too.
Thanks,
Richard
gcc/
* system.h (TEST_BIT): New macro.
* recog.h (alternative_mask): New type.
(ALL_ALTERNATIVES, ALTERNATIVE_BIT): New macros.
(recog_data_d): Replace alternative_enabled_p array with
enabled_alternatives.
(target_recog): New structure.
(default_target_recog, this_target_recog): Declare.
(get_enabled_alternatives): Likewise.
* recog.c (default_target_recog, this_target_recog): New variables.
(get_enabled_alternatives): New function.
(extract_insn): Use it.
(preprocess_constraints, constrain_operands): Adjust for change to
recog_data.
* postreload.c (reload_cse_simplify_operands): Likewise.
* reload.c (find_reloads): Likewise.
* ira-costs.c (record_reg_classes): Likewise.
* ira-lives.c (single_reg_class): Likewise. Fix bug in which
all alternatives after a disabled one would be skipped.
(ira_implicitly_set_insn_hard_regs): Likewise.
* ira.c (commutative_constraint_p): Likewise.
(ira_setup_alts): Adjust for change to recog_data.
* lra-int.h (lra_insn_recog_data): Replace alternative_enabled_p
with enabled_alternatives.
* lra.c (free_insn_recog_data): Update accordingly.
(lra_update_insn_recog_data): Likewise.
(lra_set_insn_recog_data): Likewise. Use get_enabled_alternatives.
* lra-constraints.c (process_alt_operands): Likewise. Handle
only_alternative as part of the enabled mask.
* target-globals.h (this_target_recog): Declare.
(target_globals): Add a recog field.
(restore_target_globals): Restore this_target_recog.
* target-globals.c: Include recog.h.
(default_target_globals): Initialize recog field.
(save_target_globals): 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] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-20 8:16 RFA: cache enabled attribute by insn code Richard Sandiford
@ 2014-05-20 8:17 ` Richard Sandiford
2014-05-20 14:42 ` Mike Stump
2014-05-20 17:50 ` Jeff Law
1 sibling, 1 reply; 25+ messages in thread
From: Richard Sandiford @ 2014-05-20 8:17 UTC (permalink / raw)
To: gcc-patches
Richard Sandiford <rdsandiford@googlemail.com> writes:
> get_attr_enabled was showing up high in a -O0 compile of fold-const.ii.
> At the moment, extract_insn calls this function for every alternative
> on each extraction, which can be expensive for instructions like
> moves that have many alternatives.
>
> The attribute is only supposed to depend on the insn code and the
> current target. It isn't allowed to depend on the values of operands.
> LRA already makes use of this to cache the enabled attributes based on code.
>
> This patch makes the caching compiler-wide. It also converts the bool
> array into a plain int bitfield, since at the moment we don't allow more
> than 30 alternatives. (It's possible we might want to increase it
> beyond 32 at some point, but then we can change the type to uint64_t
> instead. Wanting to go beyond 64 would suggest deeper problems
> somewhere. :-))
>
> The patch gives a consistent compile-time improvement of about ~3.5%
> on the -O0 fold-const.ii testcase.
>
> There were a few instances of the construct:
>
> cl = NO_REGS;
> for (ignore_p = false, curr_alt = 0;
> (c = *constraints);
> constraints += CONSTRAINT_LEN (c, constraints))
> if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
> ignore_p = true;
> else if (c == ',')
> {
> curr_alt++;
> ignore_p = false;
> }
> else if (! ignore_p)
>
> This had the effect of ignoring all alternatives after the first
> attribute-disabled one, since once we found a disabled alternative we'd
> always enter the first arm of the "if" and never increment curr_alt.
> I don't think that was intentional.
>
> Tested on x86_64-linux-gnu. OK to install?
>
> I notice ira_setup_alts is using a HARD_REG_SET to store a mask
> of alternatives. If this patch is OK, I'll follow up with a patch
> to use alternative_mask there too.
>
> Thanks,
> Richard
>
>
> gcc/
> * system.h (TEST_BIT): New macro.
> * recog.h (alternative_mask): New type.
> (ALL_ALTERNATIVES, ALTERNATIVE_BIT): New macros.
> (recog_data_d): Replace alternative_enabled_p array with
> enabled_alternatives.
> (target_recog): New structure.
> (default_target_recog, this_target_recog): Declare.
> (get_enabled_alternatives): Likewise.
> * recog.c (default_target_recog, this_target_recog): New variables.
> (get_enabled_alternatives): New function.
> (extract_insn): Use it.
> (preprocess_constraints, constrain_operands): Adjust for change to
> recog_data.
> * postreload.c (reload_cse_simplify_operands): Likewise.
> * reload.c (find_reloads): Likewise.
> * ira-costs.c (record_reg_classes): Likewise.
> * ira-lives.c (single_reg_class): Likewise. Fix bug in which
> all alternatives after a disabled one would be skipped.
> (ira_implicitly_set_insn_hard_regs): Likewise.
> * ira.c (commutative_constraint_p): Likewise.
> (ira_setup_alts): Adjust for change to recog_data.
> * lra-int.h (lra_insn_recog_data): Replace alternative_enabled_p
> with enabled_alternatives.
> * lra.c (free_insn_recog_data): Update accordingly.
> (lra_update_insn_recog_data): Likewise.
> (lra_set_insn_recog_data): Likewise. Use get_enabled_alternatives.
> * lra-constraints.c (process_alt_operands): Likewise. Handle
> only_alternative as part of the enabled mask.
> * target-globals.h (this_target_recog): Declare.
> (target_globals): Add a recog field.
> (restore_target_globals): Restore this_target_recog.
> * target-globals.c: Include recog.h.
> (default_target_globals): Initialize recog field.
> (save_target_globals): Likewise.
Bah, EWRONGPATCH.
Index: gcc/system.h
===================================================================
--- gcc/system.h 2014-05-20 08:23:13.770215727 +0100
+++ gcc/system.h 2014-05-20 09:14:18.343945817 +0100
@@ -1070,6 +1070,9 @@ #define DEBUG_FUNCTION
#define DEBUG_VARIABLE
#endif
+/* General macro to extract bit Y of X. */
+#define TEST_BIT(X, Y) (((X) >> (Y)) & 1)
+
/* Get definitions of HOST_WIDE_INT and HOST_WIDEST_INT. */
#include "hwint.h"
Index: gcc/recog.h
===================================================================
--- gcc/recog.h 2014-05-20 08:23:13.770215727 +0100
+++ gcc/recog.h 2014-05-20 09:14:18.344945827 +0100
@@ -20,8 +20,17 @@ Software Foundation; either version 3, o
#ifndef GCC_RECOG_H
#define GCC_RECOG_H
-/* Random number that should be large enough for all purposes. */
+/* Random number that should be large enough for all purposes. Also define
+ a type that has at least MAX_RECOG_ALTERNATIVES + 1 bits, with the extra
+ bit giving an invalid value that can be used to mean "uninitialized". */
#define MAX_RECOG_ALTERNATIVES 30
+typedef unsigned int alternative_mask;
+
+/* A mask of all alternatives. */
+#define ALL_ALTERNATIVES ((alternative_mask) -1)
+
+/* A mask containing just alternative X. */
+#define ALTERNATIVE_BIT(X) ((alternative_mask) 1 << (X))
/* Types of operands. */
enum op_type {
@@ -235,11 +244,11 @@ struct recog_data_d
/* True if insn is ASM_OPERANDS. */
bool is_asm;
- /* Specifies whether an insn alternative is enabled using the
- `enabled' attribute in the insn pattern definition. For back
- ends not using the `enabled' attribute the array fields are
- always set to `true' in expand_insn. */
- bool alternative_enabled_p [MAX_RECOG_ALTERNATIVES];
+ /* Specifies whether an insn alternative is enabled using the `enabled'
+ attribute in the insn pattern definition. For back ends not using
+ the `enabled' attribute the bits are always set to 1 in expand_insn.
+ Bits beyond the last alternative are also set to 1. */
+ alternative_mask enabled_alternatives;
/* In case we are caching, hold insn data was generated for. */
rtx insn;
@@ -361,4 +370,22 @@ struct insn_data_d
extern const struct insn_data_d insn_data[];
extern int peep2_current_count;
+#ifndef GENERATOR_FILE
+#include "insn-codes.h"
+
+/* Target-dependent globals. */
+struct target_recog {
+ alternative_mask x_enabled_alternatives[LAST_INSN_CODE];
+};
+
+extern struct target_recog default_target_recog;
+#if SWITCHABLE_TARGET
+extern struct target_recog *this_target_recog;
+#else
+#define this_target_recog (&default_target_recog)
+#endif
+
+alternative_mask get_enabled_alternatives (rtx);
+#endif
+
#endif /* GCC_RECOG_H */
Index: gcc/recog.c
===================================================================
--- gcc/recog.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/recog.c 2014-05-20 09:14:35.232103084 +0100
@@ -61,6 +61,11 @@ static void validate_replace_rtx_1 (rtx
static void validate_replace_src_1 (rtx *, void *);
static rtx split_insn (rtx);
+struct target_recog default_target_recog;
+#if SWITCHABLE_TARGET
+struct target_recog *this_target_recog = &default_target_recog;
+#endif
+
/* Nonzero means allow operands to be volatile.
This should be 0 if you are generating rtl, such as if you are calling
the functions in optabs.c and expmed.c (most of the time).
@@ -2137,6 +2142,48 @@ mode_dependent_address_p (rtx addr, addr
return targetm.mode_dependent_address_p (addr, addrspace);
}
\f
+/* Return the mask of operand alternatives that are allowed for INSN.
+ This mask depends only on INSN and on the current target; it does not
+ depend on things like the values of operands. */
+
+alternative_mask
+get_enabled_alternatives (rtx insn)
+{
+ /* Quick exit for asms and for targets that don't use the "enabled"
+ attribute. */
+ int code = INSN_CODE (insn);
+ if (code < 0 || !HAVE_ATTR_enabled)
+ return ALL_ALTERNATIVES;
+
+ /* Calling get_attr_enabled can be expensive, so cache the mask
+ for speed. */
+ if (this_target_recog->x_enabled_alternatives[code])
+ return this_target_recog->x_enabled_alternatives[code];
+
+ /* Temporarily install enough information for get_attr_enabled to assume
+ that the insn operands are already cached. As above, the attribute
+ mustn't depend on the values of operands, so we don't provide their
+ real values here. */
+ rtx old_insn = recog_data.insn;
+ int old_alternative = which_alternative;
+
+ recog_data.insn = insn;
+ alternative_mask enabled = ALL_ALTERNATIVES;
+ int n_alternatives = insn_data[code].n_alternatives;
+ for (int i = 0; i < n_alternatives; i++)
+ {
+ which_alternative = i;
+ if (!get_attr_enabled (insn))
+ enabled &= ~ALTERNATIVE_BIT (i);
+ }
+
+ recog_data.insn = old_insn;
+ which_alternative = old_alternative;
+
+ this_target_recog->x_enabled_alternatives[code] = enabled;
+ return enabled;
+}
+
/* Like extract_insn, but save insn extracted and don't extract again, when
called again for the same insn expecting that recog_data still contain the
valid information. This is used primary by gen_attr infrastructure that
@@ -2269,19 +2316,7 @@ extract_insn (rtx insn)
gcc_assert (recog_data.n_alternatives <= MAX_RECOG_ALTERNATIVES);
- if (INSN_CODE (insn) < 0)
- for (i = 0; i < recog_data.n_alternatives; i++)
- recog_data.alternative_enabled_p[i] = true;
- else
- {
- recog_data.insn = insn;
- for (i = 0; i < recog_data.n_alternatives; i++)
- {
- which_alternative = i;
- recog_data.alternative_enabled_p[i]
- = HAVE_ATTR_enabled ? get_attr_enabled (insn) : 1;
- }
- }
+ recog_data.enabled_alternatives = get_enabled_alternatives (insn);
recog_data.insn = NULL;
which_alternative = -1;
@@ -2314,7 +2349,7 @@ preprocess_constraints (void)
op_alt[j].matches = -1;
op_alt[j].matched = -1;
- if (!recog_data.alternative_enabled_p[j])
+ if (!TEST_BIT (recog_data.enabled_alternatives, j))
{
p = skip_alternative (p);
continue;
@@ -2490,7 +2525,7 @@ constrain_operands (int strict)
int lose = 0;
funny_match_index = 0;
- if (!recog_data.alternative_enabled_p[which_alternative])
+ if (!TEST_BIT (recog_data.enabled_alternatives, which_alternative))
{
int i;
Index: gcc/postreload.c
===================================================================
--- gcc/postreload.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/postreload.c 2014-05-20 09:14:18.338945771 +0100
@@ -584,7 +584,7 @@ reload_cse_simplify_operands (rtx insn,
alternative yet and the operand being replaced is not
a cheap CONST_INT. */
if (op_alt_regno[i][j] == -1
- && recog_data.alternative_enabled_p[j]
+ && TEST_BIT (recog_data.enabled_alternatives, j)
&& reg_fits_class_p (testreg, rclass, 0, mode)
&& (!CONST_INT_P (recog_data.operand[i])
|| (set_src_cost (recog_data.operand[i],
Index: gcc/reload.c
===================================================================
--- gcc/reload.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/reload.c 2014-05-20 09:14:18.342945808 +0100
@@ -3010,7 +3010,7 @@ find_reloads (rtx insn, int replace, int
{
int swapped;
- if (!recog_data.alternative_enabled_p[this_alternative_number])
+ if (!TEST_BIT (recog_data.enabled_alternatives, this_alternative_number))
{
int i;
Index: gcc/ira-costs.c
===================================================================
--- gcc/ira-costs.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/ira-costs.c 2014-05-20 09:14:18.331945705 +0100
@@ -421,7 +421,7 @@ record_reg_classes (int n_alts, int n_op
int alt_fail = 0;
int alt_cost = 0, op_cost_add;
- if (!recog_data.alternative_enabled_p[alt])
+ if (!TEST_BIT (recog_data.enabled_alternatives, alt))
{
for (i = 0; i < recog_data.n_operands; i++)
constraints[i] = skip_alternative (constraints[i]);
Index: gcc/ira-lives.c
===================================================================
--- gcc/ira-lives.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/ira-lives.c 2014-05-20 09:14:18.332945715 +0100
@@ -743,22 +743,17 @@ mark_hard_reg_early_clobbers (rtx insn,
static enum reg_class
single_reg_class (const char *constraints, rtx op, rtx equiv_const)
{
- int curr_alt, c;
- bool ignore_p;
+ int c;
enum reg_class cl, next_cl;
cl = NO_REGS;
- for (ignore_p = false, curr_alt = 0;
- (c = *constraints);
- constraints += CONSTRAINT_LEN (c, constraints))
- if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
- ignore_p = true;
+ alternative_mask enabled = recog_data.enabled_alternatives;
+ for (; (c = *constraints); constraints += CONSTRAINT_LEN (c, constraints))
+ if (c == '#')
+ enabled &= ~ALTERNATIVE_BIT (0);
else if (c == ',')
- {
- curr_alt++;
- ignore_p = false;
- }
- else if (! ignore_p)
+ enabled >>= 1;
+ else if (enabled & 1)
switch (c)
{
case ' ':
@@ -887,8 +882,7 @@ single_reg_operand_class (int op_num)
void
ira_implicitly_set_insn_hard_regs (HARD_REG_SET *set)
{
- int i, curr_alt, c, regno = 0;
- bool ignore_p;
+ int i, c, regno = 0;
enum reg_class cl;
rtx op;
enum machine_mode mode;
@@ -909,17 +903,13 @@ ira_implicitly_set_insn_hard_regs (HARD_
mode = (GET_CODE (op) == SCRATCH
? GET_MODE (op) : PSEUDO_REGNO_MODE (regno));
cl = NO_REGS;
- for (ignore_p = false, curr_alt = 0;
- (c = *p);
- p += CONSTRAINT_LEN (c, p))
- if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
- ignore_p = true;
+ alternative_mask enabled = recog_data.enabled_alternatives;
+ for (; (c = *p); p += CONSTRAINT_LEN (c, p))
+ if (c == '#')
+ enabled &= ~ALTERNATIVE_BIT (0);
else if (c == ',')
- {
- curr_alt++;
- ignore_p = false;
- }
- else if (! ignore_p)
+ enabled >>= 1;
+ else if (enabled & 1)
switch (c)
{
case 'r':
Index: gcc/ira.c
===================================================================
--- gcc/ira.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/ira.c 2014-05-20 09:14:18.334945733 +0100
@@ -1774,23 +1774,20 @@ setup_prohibited_mode_move_regs (void)
static bool
commutative_constraint_p (const char *str)
{
- int curr_alt, c;
- bool ignore_p;
+ int c;
- for (ignore_p = false, curr_alt = 0;;)
+ alternative_mask enabled = recog_data.enabled_alternatives;
+ for (;;)
{
c = *str;
if (c == '\0')
break;
str += CONSTRAINT_LEN (c, str);
- if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
- ignore_p = true;
+ if (c == '#')
+ enabled &= ~ALTERNATIVE_BIT (0);
else if (c == ',')
- {
- curr_alt++;
- ignore_p = false;
- }
- else if (! ignore_p)
+ enabled >>= 1;
+ else if (enabled & 1)
{
/* Usually `%' is the first constraint character but the
documentation does not require this. */
@@ -1844,7 +1841,8 @@ ira_setup_alts (rtx insn, HARD_REG_SET &
}
for (nalt = 0; nalt < recog_data.n_alternatives; nalt++)
{
- if (! recog_data.alternative_enabled_p[nalt] || TEST_HARD_REG_BIT (alts, nalt))
+ if (!TEST_BIT (recog_data.enabled_alternatives, nalt)
+ || TEST_HARD_REG_BIT (alts, nalt))
continue;
for (nop = 0; nop < recog_data.n_operands; nop++)
Index: gcc/lra-int.h
===================================================================
--- gcc/lra-int.h 2014-05-20 08:23:13.770215727 +0100
+++ gcc/lra-int.h 2014-05-20 09:14:18.337945761 +0100
@@ -227,7 +227,7 @@ struct lra_insn_recog_data
ending with a negative value. */
int *arg_hard_regs;
/* Alternative enabled for the insn. NULL for debug insns. */
- bool *alternative_enabled_p;
+ alternative_mask enabled_alternatives;
/* The following member value is always NULL for a debug insn. */
struct lra_insn_reg *regs;
};
Index: gcc/lra.c
===================================================================
--- gcc/lra.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/lra.c 2014-05-20 09:14:18.338945771 +0100
@@ -724,8 +724,6 @@ free_insn_recog_data (lra_insn_recog_dat
free (data->dup_loc);
if (data->arg_hard_regs != NULL)
free (data->arg_hard_regs);
- if (HAVE_ATTR_enabled && data->alternative_enabled_p != NULL)
- free (data->alternative_enabled_p);
if (data->icode < 0 && NONDEBUG_INSN_P (data->insn))
{
if (data->insn_static_data->operand_alternative != NULL)
@@ -1072,7 +1070,7 @@ lra_set_insn_recog_data (rtx insn)
data->insn_static_data = &debug_insn_static_data;
data->dup_loc = NULL;
data->arg_hard_regs = NULL;
- data->alternative_enabled_p = NULL;
+ data->enabled_alternatives = ALL_ALTERNATIVES;
data->operand_loc = XNEWVEC (rtx *, 1);
data->operand_loc[0] = &INSN_VAR_LOCATION_LOC (insn);
return data;
@@ -1132,7 +1130,7 @@ lra_set_insn_recog_data (rtx insn)
= (insn_static_data->operand[i].constraint[0] == '=' ? OP_OUT
: insn_static_data->operand[i].constraint[0] == '+' ? OP_INOUT
: OP_IN);
- data->alternative_enabled_p = NULL;
+ data->enabled_alternatives = ALL_ALTERNATIVES;
}
else
{
@@ -1159,27 +1157,7 @@ lra_set_insn_recog_data (rtx insn)
memcpy (locs, recog_data.dup_loc, n * sizeof (rtx *));
}
data->dup_loc = locs;
- if (HAVE_ATTR_enabled)
- {
- bool *bp;
-
- n = insn_static_data->n_alternatives;
- lra_assert (n >= 0);
- data->alternative_enabled_p = bp = XNEWVEC (bool, n);
- /* Cache the insn because we don't want to call extract_insn
- from get_attr_enabled as extract_insn modifies
- which_alternative. The attribute enabled should not depend
- on insn operands, operand modes, operand types, and operand
- constraints. It should depend on the architecture. If it
- is not true, we should rewrite this file code to use
- extract_insn instead of less expensive insn_extract. */
- recog_data.insn = insn;
- for (i = 0; i < n; i++)
- {
- which_alternative = i;
- bp[i] = get_attr_enabled (insn);
- }
- }
+ data->enabled_alternatives = get_enabled_alternatives (insn);
}
if (GET_CODE (PATTERN (insn)) == CLOBBER || GET_CODE (PATTERN (insn)) == USE)
insn_static_data->hard_regs = NULL;
@@ -1370,18 +1348,19 @@ lra_update_insn_recog_data (rtx insn)
#ifdef ENABLE_CHECKING
{
int i;
- bool *bp;
+ alternative_mask enabled;
n = insn_static_data->n_alternatives;
- bp = data->alternative_enabled_p;
- lra_assert (n >= 0 && bp != NULL);
+ enabled = data->enabled_alternatives;
+ lra_assert (n >= 0);
/* Cache the insn to prevent extract_insn call from
get_attr_enabled. */
recog_data.insn = insn;
for (i = 0; i < n; i++)
{
which_alternative = i;
- lra_assert (bp[i] == get_attr_enabled (insn));
+ lra_assert (TEST_BIT (enabled, i)
+ == (bool) get_attr_enabled (insn));
}
}
#endif
Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/lra-constraints.c 2014-05-20 09:14:18.336945752 +0100
@@ -1557,19 +1557,16 @@ process_alt_operands (int only_alternati
together, the second alternatives go together, etc.
First loop over alternatives. */
+ alternative_mask enabled = curr_id->enabled_alternatives;
+ if (only_alternative >= 0)
+ enabled &= ALTERNATIVE_BIT (only_alternative);
+
for (nalt = 0; nalt < n_alternatives; nalt++)
{
/* Loop over operands for one constraint alternative. */
-#if HAVE_ATTR_enabled
- if (curr_id->alternative_enabled_p != NULL
- && ! curr_id->alternative_enabled_p[nalt])
- continue;
-#endif
-
- if (only_alternative >= 0 && nalt != only_alternative)
+ if (!TEST_BIT (enabled, nalt))
continue;
-
overall = losers = reject = reload_nregs = reload_sum = 0;
for (nop = 0; nop < n_operands; nop++)
{
Index: gcc/target-globals.h
===================================================================
--- gcc/target-globals.h 2014-05-20 08:23:13.770215727 +0100
+++ gcc/target-globals.h 2014-05-20 09:14:18.343945817 +0100
@@ -24,6 +24,7 @@ #define TARGET_GLOBALS_H 1
extern struct target_flag_state *this_target_flag_state;
extern struct target_regs *this_target_regs;
extern struct target_rtl *this_target_rtl;
+extern struct target_recog *this_target_recog;
extern struct target_hard_regs *this_target_hard_regs;
extern struct target_reload *this_target_reload;
extern struct target_expmed *this_target_expmed;
@@ -43,6 +44,7 @@ struct GTY(()) target_globals {
struct target_flag_state *GTY((skip)) flag_state;
void *GTY((atomic)) regs;
struct target_rtl *rtl;
+ void *GTY((atomic)) recog;
void *GTY((atomic)) hard_regs;
void *GTY((atomic)) reload;
void *GTY((atomic)) expmed;
@@ -70,6 +72,7 @@ restore_target_globals (struct target_gl
this_target_flag_state = g->flag_state;
this_target_regs = (struct target_regs *) g->regs;
this_target_rtl = g->rtl;
+ this_target_recog = (struct target_recog *) g->recog;
this_target_hard_regs = (struct target_hard_regs *) g->hard_regs;
this_target_reload = (struct target_reload *) g->reload;
this_target_expmed = (struct target_expmed *) g->expmed;
Index: gcc/target-globals.c
===================================================================
--- gcc/target-globals.c 2014-05-20 08:23:13.770215727 +0100
+++ gcc/target-globals.c 2014-05-20 09:14:18.342945808 +0100
@@ -43,12 +43,14 @@ Software Foundation; either version 3, o
#include "gcse.h"
#include "bb-reorder.h"
#include "lower-subreg.h"
+#include "recog.h"
#if SWITCHABLE_TARGET
struct target_globals default_target_globals = {
&default_target_flag_state,
&default_target_regs,
&default_target_rtl,
+ &default_target_recog,
&default_target_hard_regs,
&default_target_reload,
&default_target_expmed,
@@ -84,6 +86,7 @@ save_target_globals (void)
g->flag_state = &p->flag_state;
g->regs = ggc_internal_cleared_alloc (sizeof (struct target_regs));
g->rtl = ggc_cleared_alloc<target_rtl> ();
+ g->recog = ggc_internal_cleared_alloc (sizeof (struct target_recog));
g->hard_regs
= ggc_internal_cleared_alloc (sizeof (struct target_hard_regs));
g->reload = ggc_internal_cleared_alloc (sizeof (struct target_reload));
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-20 8:17 ` Richard Sandiford
@ 2014-05-20 14:42 ` Mike Stump
0 siblings, 0 replies; 25+ messages in thread
From: Mike Stump @ 2014-05-20 14:42 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
On May 20, 2014, at 1:17 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>> The patch gives a consistent compile-time improvement of about ~3.5%
>> on the -O0 fold-const.ii test case.
3.5 alone is really nice and 3.5 on top of 3.5 is amazing.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-20 8:16 RFA: cache enabled attribute by insn code Richard Sandiford
2014-05-20 8:17 ` Richard Sandiford
@ 2014-05-20 17:50 ` Jeff Law
2014-05-20 21:37 ` Richard Sandiford
1 sibling, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-05-20 17:50 UTC (permalink / raw)
To: gcc-patches, rdsandiford
On 05/20/14 02:16, Richard Sandiford wrote:
> get_attr_enabled was showing up high in a -O0 compile of fold-const.ii.
> At the moment, extract_insn calls this function for every alternative
> on each extraction, which can be expensive for instructions like
> moves that have many alternatives.
>
> The attribute is only supposed to depend on the insn code and the
> current target. It isn't allowed to depend on the values of operands.
> LRA already makes use of this to cache the enabled attributes based on code.
>
> This patch makes the caching compiler-wide. It also converts the bool
> array into a plain int bitfield, since at the moment we don't allow more
> than 30 alternatives. (It's possible we might want to increase it
> beyond 32 at some point, but then we can change the type to uint64_t
> instead. Wanting to go beyond 64 would suggest deeper problems
> somewhere. :-))
>
> The patch gives a consistent compile-time improvement of about ~3.5%
> on the -O0 fold-const.ii testcase.
>
> There were a few instances of the construct:
>
> cl = NO_REGS;
> for (ignore_p = false, curr_alt = 0;
> (c = *constraints);
> constraints += CONSTRAINT_LEN (c, constraints))
> if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
> ignore_p = true;
> else if (c == ',')
> {
> curr_alt++;
> ignore_p = false;
> }
> else if (! ignore_p)
>
> This had the effect of ignoring all alternatives after the first
> attribute-disabled one, since once we found a disabled alternative we'd
> always enter the first arm of the "if" and never increment curr_alt.
> I don't think that was intentional.
>
> Tested on x86_64-linux-gnu. OK to install?
>
> I notice ira_setup_alts is using a HARD_REG_SET to store a mask
> of alternatives. If this patch is OK, I'll follow up with a patch
> to use alternative_mask there too.
>
> Thanks,
> Richard
>
>
> gcc/
> * system.h (TEST_BIT): New macro.
> * recog.h (alternative_mask): New type.
> (ALL_ALTERNATIVES, ALTERNATIVE_BIT): New macros.
> (recog_data_d): Replace alternative_enabled_p array with
> enabled_alternatives.
> (target_recog): New structure.
> (default_target_recog, this_target_recog): Declare.
> (get_enabled_alternatives): Likewise.
> * recog.c (default_target_recog, this_target_recog): New variables.
> (get_enabled_alternatives): New function.
> (extract_insn): Use it.
> (preprocess_constraints, constrain_operands): Adjust for change to
> recog_data.
> * postreload.c (reload_cse_simplify_operands): Likewise.
> * reload.c (find_reloads): Likewise.
> * ira-costs.c (record_reg_classes): Likewise.
> * ira-lives.c (single_reg_class): Likewise. Fix bug in which
> all alternatives after a disabled one would be skipped.
> (ira_implicitly_set_insn_hard_regs): Likewise.
> * ira.c (commutative_constraint_p): Likewise.
> (ira_setup_alts): Adjust for change to recog_data.
> * lra-int.h (lra_insn_recog_data): Replace alternative_enabled_p
> with enabled_alternatives.
> * lra.c (free_insn_recog_data): Update accordingly.
> (lra_update_insn_recog_data): Likewise.
> (lra_set_insn_recog_data): Likewise. Use get_enabled_alternatives.
> * lra-constraints.c (process_alt_operands): Likewise. Handle
> only_alternative as part of the enabled mask.
> * target-globals.h (this_target_recog): Declare.
> (target_globals): Add a recog field.
> (restore_target_globals): Restore this_target_recog.
> * target-globals.c: Include recog.h.
> (default_target_globals): Initialize recog field.
> (save_target_globals): Likewise.
This is OK for the trunk (referring to the follow-up message which fixed
EWRONGPATCH.
jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-20 17:50 ` Jeff Law
@ 2014-05-20 21:37 ` Richard Sandiford
2014-05-23 18:19 ` Jeff Law
0 siblings, 1 reply; 25+ messages in thread
From: Richard Sandiford @ 2014-05-20 21:37 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
Jeff Law <law@redhat.com> writes:
> On 05/20/14 02:16, Richard Sandiford wrote:
>> get_attr_enabled was showing up high in a -O0 compile of fold-const.ii.
>> At the moment, extract_insn calls this function for every alternative
>> on each extraction, which can be expensive for instructions like
>> moves that have many alternatives.
>>
>> The attribute is only supposed to depend on the insn code and the
>> current target. It isn't allowed to depend on the values of operands.
>> LRA already makes use of this to cache the enabled attributes based on code.
>>
>> This patch makes the caching compiler-wide. It also converts the bool
>> array into a plain int bitfield, since at the moment we don't allow more
>> than 30 alternatives. (It's possible we might want to increase it
>> beyond 32 at some point, but then we can change the type to uint64_t
>> instead. Wanting to go beyond 64 would suggest deeper problems
>> somewhere. :-))
>>
>> The patch gives a consistent compile-time improvement of about ~3.5%
>> on the -O0 fold-const.ii testcase.
>>
>> There were a few instances of the construct:
>>
>> cl = NO_REGS;
>> for (ignore_p = false, curr_alt = 0;
>> (c = *constraints);
>> constraints += CONSTRAINT_LEN (c, constraints))
>> if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
>> ignore_p = true;
>> else if (c == ',')
>> {
>> curr_alt++;
>> ignore_p = false;
>> }
>> else if (! ignore_p)
>>
>> This had the effect of ignoring all alternatives after the first
>> attribute-disabled one, since once we found a disabled alternative we'd
>> always enter the first arm of the "if" and never increment curr_alt.
>> I don't think that was intentional.
>>
>> Tested on x86_64-linux-gnu. OK to install?
>>
>> I notice ira_setup_alts is using a HARD_REG_SET to store a mask
>> of alternatives. If this patch is OK, I'll follow up with a patch
>> to use alternative_mask there too.
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> * system.h (TEST_BIT): New macro.
>> * recog.h (alternative_mask): New type.
>> (ALL_ALTERNATIVES, ALTERNATIVE_BIT): New macros.
>> (recog_data_d): Replace alternative_enabled_p array with
>> enabled_alternatives.
>> (target_recog): New structure.
>> (default_target_recog, this_target_recog): Declare.
>> (get_enabled_alternatives): Likewise.
>> * recog.c (default_target_recog, this_target_recog): New variables.
>> (get_enabled_alternatives): New function.
>> (extract_insn): Use it.
>> (preprocess_constraints, constrain_operands): Adjust for change to
>> recog_data.
>> * postreload.c (reload_cse_simplify_operands): Likewise.
>> * reload.c (find_reloads): Likewise.
>> * ira-costs.c (record_reg_classes): Likewise.
>> * ira-lives.c (single_reg_class): Likewise. Fix bug in which
>> all alternatives after a disabled one would be skipped.
>> (ira_implicitly_set_insn_hard_regs): Likewise.
>> * ira.c (commutative_constraint_p): Likewise.
>> (ira_setup_alts): Adjust for change to recog_data.
>> * lra-int.h (lra_insn_recog_data): Replace alternative_enabled_p
>> with enabled_alternatives.
>> * lra.c (free_insn_recog_data): Update accordingly.
>> (lra_update_insn_recog_data): Likewise.
>> (lra_set_insn_recog_data): Likewise. Use get_enabled_alternatives.
>> * lra-constraints.c (process_alt_operands): Likewise. Handle
>> only_alternative as part of the enabled mask.
>> * target-globals.h (this_target_recog): Declare.
>> (target_globals): Add a recog field.
>> (restore_target_globals): Restore this_target_recog.
>> * target-globals.c: Include recog.h.
>> (default_target_globals): Initialize recog field.
>> (save_target_globals): Likewise.
> This is OK for the trunk (referring to the follow-up message which fixed
> EWRONGPATCH.
Sorry, while working on the follow-up LRA patch, I realised I hadn't
accounted for target changes that happen directly via target_reinit
(rather than SWITCHABLE_TARGETS) and cases where reinit_regs is called
to change just the register information. Both could potentially affect
the enabled attribute.
This version adds a recog_init function that clears the data if necessary.
There are no other changes from first time. Is this still OK?
Thanks,
Richard
gcc/
* system.h (TEST_BIT): New macro.
* recog.h (alternative_mask): New type.
(ALL_ALTERNATIVES, ALTERNATIVE_BIT): New macros.
(recog_data_d): Replace alternative_enabled_p array with
enabled_alternatives.
(target_recog): New structure.
(default_target_recog, this_target_recog): Declare.
(get_enabled_alternatives, recog_init): Likewise.
* recog.c (default_target_recog, this_target_recog): New variables.
(get_enabled_alternatives): New function.
(extract_insn): Use it.
(recog_init): New function.
(preprocess_constraints, constrain_operands): Adjust for change to
recog_data.
* postreload.c (reload_cse_simplify_operands): Likewise.
* reload.c (find_reloads): Likewise.
* ira-costs.c (record_reg_classes): Likewise.
* ira-lives.c (single_reg_class): Likewise. Fix bug in which
all alternatives after a disabled one would be skipped.
(ira_implicitly_set_insn_hard_regs): Likewise.
* ira.c (commutative_constraint_p): Likewise.
(ira_setup_alts): Adjust for change to recog_data.
* lra-int.h (lra_insn_recog_data): Replace alternative_enabled_p
with enabled_alternatives.
* lra.c (free_insn_recog_data): Update accordingly.
(lra_update_insn_recog_data): Likewise.
(lra_set_insn_recog_data): Likewise. Use get_enabled_alternatives.
* lra-constraints.c (process_alt_operands): Likewise. Handle
only_alternative as part of the enabled mask.
* target-globals.h (this_target_recog): Declare.
(target_globals): Add a recog field.
(restore_target_globals): Restore this_target_recog.
* target-globals.c: Include recog.h.
(default_target_globals): Initialize recog field.
(save_target_globals): Likewise.
* reginfo.c (reinit_regs): Call recog_init.
* toplev.c (backend_init_target): Likewise.
Index: gcc/system.h
===================================================================
--- gcc/system.h 2014-05-20 22:18:09.273472110 +0100
+++ gcc/system.h 2014-05-20 22:34:38.136658465 +0100
@@ -1070,6 +1070,9 @@ #define DEBUG_FUNCTION
#define DEBUG_VARIABLE
#endif
+/* General macro to extract bit Y of X. */
+#define TEST_BIT(X, Y) (((X) >> (Y)) & 1)
+
/* Get definitions of HOST_WIDE_INT and HOST_WIDEST_INT. */
#include "hwint.h"
Index: gcc/recog.h
===================================================================
--- gcc/recog.h 2014-05-20 22:18:09.273472110 +0100
+++ gcc/recog.h 2014-05-20 22:34:38.137658475 +0100
@@ -20,8 +20,17 @@ Software Foundation; either version 3, o
#ifndef GCC_RECOG_H
#define GCC_RECOG_H
-/* Random number that should be large enough for all purposes. */
+/* Random number that should be large enough for all purposes. Also define
+ a type that has at least MAX_RECOG_ALTERNATIVES + 1 bits, with the extra
+ bit giving an invalid value that can be used to mean "uninitialized". */
#define MAX_RECOG_ALTERNATIVES 30
+typedef unsigned int alternative_mask;
+
+/* A mask of all alternatives. */
+#define ALL_ALTERNATIVES ((alternative_mask) -1)
+
+/* A mask containing just alternative X. */
+#define ALTERNATIVE_BIT(X) ((alternative_mask) 1 << (X))
/* Types of operands. */
enum op_type {
@@ -235,11 +244,11 @@ struct recog_data_d
/* True if insn is ASM_OPERANDS. */
bool is_asm;
- /* Specifies whether an insn alternative is enabled using the
- `enabled' attribute in the insn pattern definition. For back
- ends not using the `enabled' attribute the array fields are
- always set to `true' in expand_insn. */
- bool alternative_enabled_p [MAX_RECOG_ALTERNATIVES];
+ /* Specifies whether an insn alternative is enabled using the `enabled'
+ attribute in the insn pattern definition. For back ends not using
+ the `enabled' attribute the bits are always set to 1 in expand_insn.
+ Bits beyond the last alternative are also set to 1. */
+ alternative_mask enabled_alternatives;
/* In case we are caching, hold insn data was generated for. */
rtx insn;
@@ -361,4 +370,25 @@ struct insn_data_d
extern const struct insn_data_d insn_data[];
extern int peep2_current_count;
+#ifndef GENERATOR_FILE
+#include "insn-codes.h"
+
+/* Target-dependent globals. */
+struct target_recog {
+ bool x_initialized;
+ alternative_mask x_enabled_alternatives[LAST_INSN_CODE];
+};
+
+extern struct target_recog default_target_recog;
+#if SWITCHABLE_TARGET
+extern struct target_recog *this_target_recog;
+#else
+#define this_target_recog (&default_target_recog)
+#endif
+
+alternative_mask get_enabled_alternatives (rtx);
+
+void recog_init ();
+#endif
+
#endif /* GCC_RECOG_H */
Index: gcc/recog.c
===================================================================
--- gcc/recog.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/recog.c 2014-05-20 22:34:45.658731000 +0100
@@ -61,6 +61,11 @@ static void validate_replace_rtx_1 (rtx
static void validate_replace_src_1 (rtx *, void *);
static rtx split_insn (rtx);
+struct target_recog default_target_recog;
+#if SWITCHABLE_TARGET
+struct target_recog *this_target_recog = &default_target_recog;
+#endif
+
/* Nonzero means allow operands to be volatile.
This should be 0 if you are generating rtl, such as if you are calling
the functions in optabs.c and expmed.c (most of the time).
@@ -2137,6 +2142,48 @@ mode_dependent_address_p (rtx addr, addr
return targetm.mode_dependent_address_p (addr, addrspace);
}
\f
+/* Return the mask of operand alternatives that are allowed for INSN.
+ This mask depends only on INSN and on the current target; it does not
+ depend on things like the values of operands. */
+
+alternative_mask
+get_enabled_alternatives (rtx insn)
+{
+ /* Quick exit for asms and for targets that don't use the "enabled"
+ attribute. */
+ int code = INSN_CODE (insn);
+ if (code < 0 || !HAVE_ATTR_enabled)
+ return ALL_ALTERNATIVES;
+
+ /* Calling get_attr_enabled can be expensive, so cache the mask
+ for speed. */
+ if (this_target_recog->x_enabled_alternatives[code])
+ return this_target_recog->x_enabled_alternatives[code];
+
+ /* Temporarily install enough information for get_attr_enabled to assume
+ that the insn operands are already cached. As above, the attribute
+ mustn't depend on the values of operands, so we don't provide their
+ real values here. */
+ rtx old_insn = recog_data.insn;
+ int old_alternative = which_alternative;
+
+ recog_data.insn = insn;
+ alternative_mask enabled = ALL_ALTERNATIVES;
+ int n_alternatives = insn_data[code].n_alternatives;
+ for (int i = 0; i < n_alternatives; i++)
+ {
+ which_alternative = i;
+ if (!get_attr_enabled (insn))
+ enabled &= ~ALTERNATIVE_BIT (i);
+ }
+
+ recog_data.insn = old_insn;
+ which_alternative = old_alternative;
+
+ this_target_recog->x_enabled_alternatives[code] = enabled;
+ return enabled;
+}
+
/* Like extract_insn, but save insn extracted and don't extract again, when
called again for the same insn expecting that recog_data still contain the
valid information. This is used primary by gen_attr infrastructure that
@@ -2269,19 +2316,7 @@ extract_insn (rtx insn)
gcc_assert (recog_data.n_alternatives <= MAX_RECOG_ALTERNATIVES);
- if (INSN_CODE (insn) < 0)
- for (i = 0; i < recog_data.n_alternatives; i++)
- recog_data.alternative_enabled_p[i] = true;
- else
- {
- recog_data.insn = insn;
- for (i = 0; i < recog_data.n_alternatives; i++)
- {
- which_alternative = i;
- recog_data.alternative_enabled_p[i]
- = HAVE_ATTR_enabled ? get_attr_enabled (insn) : 1;
- }
- }
+ recog_data.enabled_alternatives = get_enabled_alternatives (insn);
recog_data.insn = NULL;
which_alternative = -1;
@@ -2314,7 +2349,7 @@ preprocess_constraints (void)
op_alt[j].matches = -1;
op_alt[j].matched = -1;
- if (!recog_data.alternative_enabled_p[j])
+ if (!TEST_BIT (recog_data.enabled_alternatives, j))
{
p = skip_alternative (p);
continue;
@@ -2490,7 +2525,7 @@ constrain_operands (int strict)
int lose = 0;
funny_match_index = 0;
- if (!recog_data.alternative_enabled_p[which_alternative])
+ if (!TEST_BIT (recog_data.enabled_alternatives, which_alternative))
{
int i;
@@ -4164,3 +4199,19 @@ make_pass_split_for_shorten_branches (gc
{
return new pass_split_for_shorten_branches (ctxt);
}
+
+/* (Re)initialize the target information after a change in target. */
+
+void
+recog_init ()
+{
+ /* The information is zero-initialized, so we don't need to do anything
+ first time round. */
+ if (!this_target_recog->x_initialized)
+ {
+ this_target_recog->x_initialized = true;
+ return;
+ }
+ memset (this_target_recog->x_enabled_alternatives, 0,
+ sizeof (this_target_recog->x_enabled_alternatives));
+}
Index: gcc/postreload.c
===================================================================
--- gcc/postreload.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/postreload.c 2014-05-20 22:34:38.131658417 +0100
@@ -584,7 +584,7 @@ reload_cse_simplify_operands (rtx insn,
alternative yet and the operand being replaced is not
a cheap CONST_INT. */
if (op_alt_regno[i][j] == -1
- && recog_data.alternative_enabled_p[j]
+ && TEST_BIT (recog_data.enabled_alternatives, j)
&& reg_fits_class_p (testreg, rclass, 0, mode)
&& (!CONST_INT_P (recog_data.operand[i])
|| (set_src_cost (recog_data.operand[i],
Index: gcc/reload.c
===================================================================
--- gcc/reload.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/reload.c 2014-05-20 22:34:38.135658456 +0100
@@ -3010,7 +3010,7 @@ find_reloads (rtx insn, int replace, int
{
int swapped;
- if (!recog_data.alternative_enabled_p[this_alternative_number])
+ if (!TEST_BIT (recog_data.enabled_alternatives, this_alternative_number))
{
int i;
Index: gcc/ira-costs.c
===================================================================
--- gcc/ira-costs.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/ira-costs.c 2014-05-20 22:34:38.124658350 +0100
@@ -421,7 +421,7 @@ record_reg_classes (int n_alts, int n_op
int alt_fail = 0;
int alt_cost = 0, op_cost_add;
- if (!recog_data.alternative_enabled_p[alt])
+ if (!TEST_BIT (recog_data.enabled_alternatives, alt))
{
for (i = 0; i < recog_data.n_operands; i++)
constraints[i] = skip_alternative (constraints[i]);
Index: gcc/ira-lives.c
===================================================================
--- gcc/ira-lives.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/ira-lives.c 2014-05-20 22:34:38.125658359 +0100
@@ -743,22 +743,17 @@ mark_hard_reg_early_clobbers (rtx insn,
static enum reg_class
single_reg_class (const char *constraints, rtx op, rtx equiv_const)
{
- int curr_alt, c;
- bool ignore_p;
+ int c;
enum reg_class cl, next_cl;
cl = NO_REGS;
- for (ignore_p = false, curr_alt = 0;
- (c = *constraints);
- constraints += CONSTRAINT_LEN (c, constraints))
- if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
- ignore_p = true;
+ alternative_mask enabled = recog_data.enabled_alternatives;
+ for (; (c = *constraints); constraints += CONSTRAINT_LEN (c, constraints))
+ if (c == '#')
+ enabled &= ~ALTERNATIVE_BIT (0);
else if (c == ',')
- {
- curr_alt++;
- ignore_p = false;
- }
- else if (! ignore_p)
+ enabled >>= 1;
+ else if (enabled & 1)
switch (c)
{
case ' ':
@@ -887,8 +882,7 @@ single_reg_operand_class (int op_num)
void
ira_implicitly_set_insn_hard_regs (HARD_REG_SET *set)
{
- int i, curr_alt, c, regno = 0;
- bool ignore_p;
+ int i, c, regno = 0;
enum reg_class cl;
rtx op;
enum machine_mode mode;
@@ -909,17 +903,13 @@ ira_implicitly_set_insn_hard_regs (HARD_
mode = (GET_CODE (op) == SCRATCH
? GET_MODE (op) : PSEUDO_REGNO_MODE (regno));
cl = NO_REGS;
- for (ignore_p = false, curr_alt = 0;
- (c = *p);
- p += CONSTRAINT_LEN (c, p))
- if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
- ignore_p = true;
+ alternative_mask enabled = recog_data.enabled_alternatives;
+ for (; (c = *p); p += CONSTRAINT_LEN (c, p))
+ if (c == '#')
+ enabled &= ~ALTERNATIVE_BIT (0);
else if (c == ',')
- {
- curr_alt++;
- ignore_p = false;
- }
- else if (! ignore_p)
+ enabled >>= 1;
+ else if (enabled & 1)
switch (c)
{
case 'r':
Index: gcc/ira.c
===================================================================
--- gcc/ira.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/ira.c 2014-05-20 22:34:38.127658379 +0100
@@ -1774,23 +1774,20 @@ setup_prohibited_mode_move_regs (void)
static bool
commutative_constraint_p (const char *str)
{
- int curr_alt, c;
- bool ignore_p;
+ int c;
- for (ignore_p = false, curr_alt = 0;;)
+ alternative_mask enabled = recog_data.enabled_alternatives;
+ for (;;)
{
c = *str;
if (c == '\0')
break;
str += CONSTRAINT_LEN (c, str);
- if (c == '#' || !recog_data.alternative_enabled_p[curr_alt])
- ignore_p = true;
+ if (c == '#')
+ enabled &= ~ALTERNATIVE_BIT (0);
else if (c == ',')
- {
- curr_alt++;
- ignore_p = false;
- }
- else if (! ignore_p)
+ enabled >>= 1;
+ else if (enabled & 1)
{
/* Usually `%' is the first constraint character but the
documentation does not require this. */
@@ -1844,7 +1841,8 @@ ira_setup_alts (rtx insn, HARD_REG_SET &
}
for (nalt = 0; nalt < recog_data.n_alternatives; nalt++)
{
- if (! recog_data.alternative_enabled_p[nalt] || TEST_HARD_REG_BIT (alts, nalt))
+ if (!TEST_BIT (recog_data.enabled_alternatives, nalt)
+ || TEST_HARD_REG_BIT (alts, nalt))
continue;
for (nop = 0; nop < recog_data.n_operands; nop++)
Index: gcc/lra-int.h
===================================================================
--- gcc/lra-int.h 2014-05-20 22:18:09.273472110 +0100
+++ gcc/lra-int.h 2014-05-20 22:34:38.129658398 +0100
@@ -227,7 +227,7 @@ struct lra_insn_recog_data
ending with a negative value. */
int *arg_hard_regs;
/* Alternative enabled for the insn. NULL for debug insns. */
- bool *alternative_enabled_p;
+ alternative_mask enabled_alternatives;
/* The following member value is always NULL for a debug insn. */
struct lra_insn_reg *regs;
};
Index: gcc/lra.c
===================================================================
--- gcc/lra.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/lra.c 2014-05-20 22:34:38.130658408 +0100
@@ -724,8 +724,6 @@ free_insn_recog_data (lra_insn_recog_dat
free (data->dup_loc);
if (data->arg_hard_regs != NULL)
free (data->arg_hard_regs);
- if (HAVE_ATTR_enabled && data->alternative_enabled_p != NULL)
- free (data->alternative_enabled_p);
if (data->icode < 0 && NONDEBUG_INSN_P (data->insn))
{
if (data->insn_static_data->operand_alternative != NULL)
@@ -1072,7 +1070,7 @@ lra_set_insn_recog_data (rtx insn)
data->insn_static_data = &debug_insn_static_data;
data->dup_loc = NULL;
data->arg_hard_regs = NULL;
- data->alternative_enabled_p = NULL;
+ data->enabled_alternatives = ALL_ALTERNATIVES;
data->operand_loc = XNEWVEC (rtx *, 1);
data->operand_loc[0] = &INSN_VAR_LOCATION_LOC (insn);
return data;
@@ -1132,7 +1130,7 @@ lra_set_insn_recog_data (rtx insn)
= (insn_static_data->operand[i].constraint[0] == '=' ? OP_OUT
: insn_static_data->operand[i].constraint[0] == '+' ? OP_INOUT
: OP_IN);
- data->alternative_enabled_p = NULL;
+ data->enabled_alternatives = ALL_ALTERNATIVES;
}
else
{
@@ -1159,27 +1157,7 @@ lra_set_insn_recog_data (rtx insn)
memcpy (locs, recog_data.dup_loc, n * sizeof (rtx *));
}
data->dup_loc = locs;
- if (HAVE_ATTR_enabled)
- {
- bool *bp;
-
- n = insn_static_data->n_alternatives;
- lra_assert (n >= 0);
- data->alternative_enabled_p = bp = XNEWVEC (bool, n);
- /* Cache the insn because we don't want to call extract_insn
- from get_attr_enabled as extract_insn modifies
- which_alternative. The attribute enabled should not depend
- on insn operands, operand modes, operand types, and operand
- constraints. It should depend on the architecture. If it
- is not true, we should rewrite this file code to use
- extract_insn instead of less expensive insn_extract. */
- recog_data.insn = insn;
- for (i = 0; i < n; i++)
- {
- which_alternative = i;
- bp[i] = get_attr_enabled (insn);
- }
- }
+ data->enabled_alternatives = get_enabled_alternatives (insn);
}
if (GET_CODE (PATTERN (insn)) == CLOBBER || GET_CODE (PATTERN (insn)) == USE)
insn_static_data->hard_regs = NULL;
@@ -1370,18 +1348,19 @@ lra_update_insn_recog_data (rtx insn)
#ifdef ENABLE_CHECKING
{
int i;
- bool *bp;
+ alternative_mask enabled;
n = insn_static_data->n_alternatives;
- bp = data->alternative_enabled_p;
- lra_assert (n >= 0 && bp != NULL);
+ enabled = data->enabled_alternatives;
+ lra_assert (n >= 0);
/* Cache the insn to prevent extract_insn call from
get_attr_enabled. */
recog_data.insn = insn;
for (i = 0; i < n; i++)
{
which_alternative = i;
- lra_assert (bp[i] == get_attr_enabled (insn));
+ lra_assert (TEST_BIT (enabled, i)
+ == (bool) get_attr_enabled (insn));
}
}
#endif
Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/lra-constraints.c 2014-05-20 22:34:38.129658398 +0100
@@ -1557,19 +1557,16 @@ process_alt_operands (int only_alternati
together, the second alternatives go together, etc.
First loop over alternatives. */
+ alternative_mask enabled = curr_id->enabled_alternatives;
+ if (only_alternative >= 0)
+ enabled &= ALTERNATIVE_BIT (only_alternative);
+
for (nalt = 0; nalt < n_alternatives; nalt++)
{
/* Loop over operands for one constraint alternative. */
-#if HAVE_ATTR_enabled
- if (curr_id->alternative_enabled_p != NULL
- && ! curr_id->alternative_enabled_p[nalt])
- continue;
-#endif
-
- if (only_alternative >= 0 && nalt != only_alternative)
+ if (!TEST_BIT (enabled, nalt))
continue;
-
overall = losers = reject = reload_nregs = reload_sum = 0;
for (nop = 0; nop < n_operands; nop++)
{
Index: gcc/target-globals.h
===================================================================
--- gcc/target-globals.h 2014-05-20 22:18:09.273472110 +0100
+++ gcc/target-globals.h 2014-05-20 22:34:38.136658465 +0100
@@ -24,6 +24,7 @@ #define TARGET_GLOBALS_H 1
extern struct target_flag_state *this_target_flag_state;
extern struct target_regs *this_target_regs;
extern struct target_rtl *this_target_rtl;
+extern struct target_recog *this_target_recog;
extern struct target_hard_regs *this_target_hard_regs;
extern struct target_reload *this_target_reload;
extern struct target_expmed *this_target_expmed;
@@ -43,6 +44,7 @@ struct GTY(()) target_globals {
struct target_flag_state *GTY((skip)) flag_state;
void *GTY((atomic)) regs;
struct target_rtl *rtl;
+ void *GTY((atomic)) recog;
void *GTY((atomic)) hard_regs;
void *GTY((atomic)) reload;
void *GTY((atomic)) expmed;
@@ -70,6 +72,7 @@ restore_target_globals (struct target_gl
this_target_flag_state = g->flag_state;
this_target_regs = (struct target_regs *) g->regs;
this_target_rtl = g->rtl;
+ this_target_recog = (struct target_recog *) g->recog;
this_target_hard_regs = (struct target_hard_regs *) g->hard_regs;
this_target_reload = (struct target_reload *) g->reload;
this_target_expmed = (struct target_expmed *) g->expmed;
Index: gcc/target-globals.c
===================================================================
--- gcc/target-globals.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/target-globals.c 2014-05-20 22:34:38.135658456 +0100
@@ -43,12 +43,14 @@ Software Foundation; either version 3, o
#include "gcse.h"
#include "bb-reorder.h"
#include "lower-subreg.h"
+#include "recog.h"
#if SWITCHABLE_TARGET
struct target_globals default_target_globals = {
&default_target_flag_state,
&default_target_regs,
&default_target_rtl,
+ &default_target_recog,
&default_target_hard_regs,
&default_target_reload,
&default_target_expmed,
@@ -84,6 +86,7 @@ save_target_globals (void)
g->flag_state = &p->flag_state;
g->regs = ggc_internal_cleared_alloc (sizeof (struct target_regs));
g->rtl = ggc_cleared_alloc<target_rtl> ();
+ g->recog = ggc_internal_cleared_alloc (sizeof (struct target_recog));
g->hard_regs
= ggc_internal_cleared_alloc (sizeof (struct target_hard_regs));
g->reload = ggc_internal_cleared_alloc (sizeof (struct target_reload));
Index: gcc/reginfo.c
===================================================================
--- gcc/reginfo.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/reginfo.c 2014-05-20 22:34:38.137658475 +0100
@@ -534,6 +534,7 @@ reinit_regs (void)
/* caller_save needs to be re-initialized. */
caller_save_initialized_p = false;
ira_init ();
+ recog_init ();
}
/* Initialize some fake stack-frame MEM references for use in
Index: gcc/toplev.c
===================================================================
--- gcc/toplev.c 2014-05-20 22:18:09.273472110 +0100
+++ gcc/toplev.c 2014-05-20 22:34:38.138658485 +0100
@@ -1601,6 +1601,9 @@ backend_init_target (void)
/* Depends on HARD_FRAME_POINTER_REGNUM. */
init_reload ();
+ /* Depends on the enabled attribute. */
+ recog_init ();
+
/* The following initialization functions need to generate rtl, so
provide a dummy function context for them. */
init_dummy_function_start ();
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-20 21:37 ` Richard Sandiford
@ 2014-05-23 18:19 ` Jeff Law
2014-05-27 13:12 ` Christophe Lyon
0 siblings, 1 reply; 25+ messages in thread
From: Jeff Law @ 2014-05-23 18:19 UTC (permalink / raw)
To: gcc-patches, rdsandiford
On 05/20/14 15:36, Richard Sandiford wrote:
>> This is OK for the trunk (referring to the follow-up message which fixed
>> EWRONGPATCH.
>
> Sorry, while working on the follow-up LRA patch, I realised I hadn't
> accounted for target changes that happen directly via target_reinit
> (rather than SWITCHABLE_TARGETS) and cases where reinit_regs is called
> to change just the register information. Both could potentially affect
> the enabled attribute.
>
> This version adds a recog_init function that clears the data if necessary.
> There are no other changes from first time. Is this still OK?
Thanks for letting me know, that's a minor twiddle -- the patch is still OK.
Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-23 18:19 ` Jeff Law
@ 2014-05-27 13:12 ` Christophe Lyon
2014-05-27 13:37 ` Ramana Radhakrishnan
2014-05-27 14:08 ` Richard Sandiford
0 siblings, 2 replies; 25+ messages in thread
From: Christophe Lyon @ 2014-05-27 13:12 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, rdsandiford
Hi,
Commits 210964 and 210965 for this patch have broken GCC build on arm* targets.
For instance on target arm-none-linux-gnueabi, when creating
unwind-arm.o, I can see:
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:1362
0x8e3bcd process_insn_for_elimination
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1344
0x8e3bcd lra_eliminate(bool, bool)
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1408
0x8dd753 lra_constraints(bool)
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-constraints.c:4049
0x8cdc1a lra(_IO_FILE*)
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:2332
0x8911f8 do_reload
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5444
0x891508 execute
/tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5605
Please submit a full bug report,
On 23 May 2014 20:19, Jeff Law <law@redhat.com> wrote:
> On 05/20/14 15:36, Richard Sandiford wrote:
>>>
>>> This is OK for the trunk (referring to the follow-up message which fixed
>>> EWRONGPATCH.
>>
>>
>> Sorry, while working on the follow-up LRA patch, I realised I hadn't
>> accounted for target changes that happen directly via target_reinit
>> (rather than SWITCHABLE_TARGETS) and cases where reinit_regs is called
>> to change just the register information. Both could potentially affect
>> the enabled attribute.
>>
>> This version adds a recog_init function that clears the data if necessary.
>> There are no other changes from first time. Is this still OK?
>
> Thanks for letting me know, that's a minor twiddle -- the patch is still OK.
>
> Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 13:12 ` Christophe Lyon
@ 2014-05-27 13:37 ` Ramana Radhakrishnan
2014-05-27 14:59 ` Christophe Lyon
2014-05-27 14:08 ` Richard Sandiford
1 sibling, 1 reply; 25+ messages in thread
From: Ramana Radhakrishnan @ 2014-05-27 13:37 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Jeff Law, gcc-patches, Richard Sandiford
On Tue, May 27, 2014 at 2:12 PM, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> Hi,
>
> Commits 210964 and 210965 for this patch have broken GCC build on arm* targets.
> For instance on target arm-none-linux-gnueabi, when creating
> unwind-arm.o, I can see:
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:1362
> 0x8e3bcd process_insn_for_elimination
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1344
> 0x8e3bcd lra_eliminate(bool, bool)
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1408
> 0x8dd753 lra_constraints(bool)
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-constraints.c:4049
> 0x8cdc1a lra(_IO_FILE*)
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:2332
> 0x8911f8 do_reload
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5444
> 0x891508 execute
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5605
> Please submit a full bug report,
>
>
Can you file a proper bug report with a pre-processed file -
information about configury options etc. to show what the issue is.
Ramana
>
>
> On 23 May 2014 20:19, Jeff Law <law@redhat.com> wrote:
>> On 05/20/14 15:36, Richard Sandiford wrote:
>>>>
>>>> This is OK for the trunk (referring to the follow-up message which fixed
>>>> EWRONGPATCH.
>>>
>>>
>>> Sorry, while working on the follow-up LRA patch, I realised I hadn't
>>> accounted for target changes that happen directly via target_reinit
>>> (rather than SWITCHABLE_TARGETS) and cases where reinit_regs is called
>>> to change just the register information. Both could potentially affect
>>> the enabled attribute.
>>>
>>> This version adds a recog_init function that clears the data if necessary.
>>> There are no other changes from first time. Is this still OK?
>>
>> Thanks for letting me know, that's a minor twiddle -- the patch is still OK.
>>
>> Jeff
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 13:12 ` Christophe Lyon
2014-05-27 13:37 ` Ramana Radhakrishnan
@ 2014-05-27 14:08 ` Richard Sandiford
2014-05-27 14:18 ` Richard Sandiford
2014-05-27 15:15 ` Richard Earnshaw
1 sibling, 2 replies; 25+ messages in thread
From: Richard Sandiford @ 2014-05-27 14:08 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Jeff Law, gcc-patches
Christophe Lyon <christophe.lyon@linaro.org> writes:
> Hi,
>
> Commits 210964 and 210965 for this patch have broken GCC build on arm* targets.
Could you send me the .i?
> For instance on target arm-none-linux-gnueabi, when creating
> unwind-arm.o, I can see:
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:1362
> 0x8e3bcd process_insn_for_elimination
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1344
> 0x8e3bcd lra_eliminate(bool, bool)
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1408
> 0x8dd753 lra_constraints(bool)
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-constraints.c:4049
> 0x8cdc1a lra(_IO_FILE*)
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:2332
> 0x8911f8 do_reload
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5444
> 0x891508 execute
> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5605
> Please submit a full bug report,
Hmm, is this because of "insn_enabled"? If so, how did that work before
the patch? LRA already assumed that the "enabled" attribute didn't depend
on the operands.
Does the following patch help?
Thanks,
Richard
gcc/
* config/arm/iterators.md (shiftable_ops): New code iterator.
(t2_binop0): New code attribute.
* config/arm/arm.md (insn_enabled): Delete.
(enabled): Remove insn_enabled test.
(*arith_shiftsi): Split out...
(*arith_multsi): ...this pattern and remove insn_enabled attribute.
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md (revision 210972)
+++ gcc/config/arm/arm.md (working copy)
@@ -200,17 +200,9 @@
(const_string "yes")]
(const_string "no")))
-; Allows an insn to disable certain alternatives for reasons other than
-; arch support.
-(define_attr "insn_enabled" "no,yes"
- (const_string "yes"))
-
; Enable all alternatives that are both arch_enabled and insn_enabled.
(define_attr "enabled" "no,yes"
- (cond [(eq_attr "insn_enabled" "no")
- (const_string "no")
-
- (and (eq_attr "predicable_short_it" "no")
+ (cond [(and (eq_attr "predicable_short_it" "no")
(and (eq_attr "predicated" "yes")
(match_test "arm_restrict_it")))
(const_string "no")
@@ -9876,39 +9868,38 @@
\f
;; Patterns to allow combination of arithmetic, cond code and shifts
-(define_insn "*arith_shiftsi"
- [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
- (match_operator:SI 1 "shiftable_operator"
- [(match_operator:SI 3 "shift_operator"
- [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
- (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
- (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
+;; Separate out the (mult ...) case, since it doesn't allow register operands.
+(define_insn "*arith_multsi"
+ [(set (match_operand:SI 0 "s_register_operand" "=r,r")
+ (shiftable_ops:SI
+ (match_operator:SI 2 "mult_operator"
+ [(match_operand:SI 3 "s_register_operand" "r,r")
+ (match_operand:SI 4 "power_of_two_operand")])
+ (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
"TARGET_32BIT"
- "%i1%?\\t%0, %2, %4%S3"
+ "%i1%?\\t%0, %1, %3%S2"
[(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "no")
(set_attr "shift" "4")
- (set_attr "arch" "a,t2,t2,a")
- ;; Thumb2 doesn't allow the stack pointer to be used for
- ;; operand1 for all operations other than add and sub. In this case
- ;; the minus operation is a candidate for an rsub and hence needs
- ;; to be disabled.
- ;; We have to make sure to disable the fourth alternative if
- ;; the shift_operator is MULT, since otherwise the insn will
- ;; also match a multiply_accumulate pattern and validate_change
- ;; will allow a replacement of the constant with a register
- ;; despite the checks done in shift_operator.
- (set_attr_alternative "insn_enabled"
- [(const_string "yes")
- (if_then_else
- (match_operand:SI 1 "add_operator" "")
- (const_string "yes") (const_string "no"))
- (const_string "yes")
- (if_then_else
- (match_operand:SI 3 "mult_operator" "")
- (const_string "no") (const_string "yes"))])
- (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
+ (set_attr "arch" "a,t2")
+ (set_attr "type" "alu_shift_imm,alu_shift_imm")])
+;; Handles cases other than the above.
+(define_insn "*arith_shiftsi"
+ [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
+ (shiftable_ops:SI
+ (match_operator:SI 2 "shift_operator"
+ [(match_operand:SI 3 "s_register_operand" "r,r,r")
+ (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
+ (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
+ "TARGET_32BIT && GET_CODE (operands[3]) != MULT"
+ "%i1%?\\t%0, %1, %3%S2"
+ [(set_attr "predicable" "yes")
+ (set_attr "predicable_short_it" "no")
+ (set_attr "shift" "4")
+ (set_attr "arch" "a,t2,a")
+ (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
+
(define_split
[(set (match_operand:SI 0 "s_register_operand" "")
(match_operator:SI 1 "shiftable_operator"
Index: gcc/config/arm/iterators.md
===================================================================
--- gcc/config/arm/iterators.md (revision 210972)
+++ gcc/config/arm/iterators.md (working copy)
@@ -194,6 +194,15 @@
;; Right shifts
(define_code_iterator rshifts [ashiftrt lshiftrt])
+;; Binary operators whose second operand can be shifted.
+(define_code_iterator shiftable_ops [plus minus ior xor and])
+
+;; plus and minus are the only shiftable_ops for which Thumb2 allows
+;; a stack pointer opoerand. The minus operation is a candidate for an rsub
+;; and hence only plus is supported.
+(define_code_attr t2_binop0
+ [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
+
;;----------------------------------------------------------------------------
;; Int iterators
;;----------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 14:08 ` Richard Sandiford
@ 2014-05-27 14:18 ` Richard Sandiford
2014-05-27 15:07 ` Richard Sandiford
2014-05-27 15:15 ` Richard Earnshaw
1 sibling, 1 reply; 25+ messages in thread
From: Richard Sandiford @ 2014-05-27 14:18 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Jeff Law, gcc-patches
Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
> Does the following patch help?
Bah, it won't of course: %i1 needs to be the operator.
Richard
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 13:37 ` Ramana Radhakrishnan
@ 2014-05-27 14:59 ` Christophe Lyon
0 siblings, 0 replies; 25+ messages in thread
From: Christophe Lyon @ 2014-05-27 14:59 UTC (permalink / raw)
To: Ramana Radhakrishnan; +Cc: Jeff Law, gcc-patches, Richard Sandiford
On 27 May 2014 15:37, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote:
> On Tue, May 27, 2014 at 2:12 PM, Christophe Lyon
> <christophe.lyon@linaro.org> wrote:
>> Hi,
>>
>> Commits 210964 and 210965 for this patch have broken GCC build on arm* targets.
>> For instance on target arm-none-linux-gnueabi, when creating
>> unwind-arm.o, I can see:
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:1362
>> 0x8e3bcd process_insn_for_elimination
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1344
>> 0x8e3bcd lra_eliminate(bool, bool)
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1408
>> 0x8dd753 lra_constraints(bool)
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-constraints.c:4049
>> 0x8cdc1a lra(_IO_FILE*)
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:2332
>> 0x8911f8 do_reload
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5444
>> 0x891508 execute
>> /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5605
>> Please submit a full bug report,
>>
>>
>
> Can you file a proper bug report with a pre-processed file -
> information about configury options etc. to show what the issue is.
>
Filed PR 61331.
Thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 14:18 ` Richard Sandiford
@ 2014-05-27 15:07 ` Richard Sandiford
2014-05-27 16:36 ` Christophe Lyon
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Richard Sandiford @ 2014-05-27 15:07 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Jeff Law, gcc-patches
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
>> Does the following patch help?
>
> Bah, it won't of course: %i1 needs to be the operator.
Here's v2. I tested that it worked for simple tests like:
int f1 (int x, int y) { return x + (y << 4); }
int f2 (int x, int y) { return x - (y << 4); }
int f3 (int x, int y) { return x & (y << 4); }
int f4 (int x, int y) { return x | (y << 4); }
int f5 (int x, int y) { return x ^ (y << 4); }
int f6 (int x, int y) { return (y << 4) - x; }
int g1 (int x, int y, int z) { return x + (y << z); }
int g2 (int x, int y, int z) { return x - (y << z); }
int g3 (int x, int y, int z) { return x & (y << z); }
int g4 (int x, int y, int z) { return x | (y << z); }
int g5 (int x, int y, int z) { return x ^ (y << z); }
int g6 (int x, int y, int z) { return (y << z) - x; }
as well as the testcase.
Thanks,
Richard
gcc/
* config/arm/iterators.md (shiftable_ops): New code iterator.
(t2_binop0, arith_shift_insn): New code attributes.
* config/arm/arm.md (insn_enabled): Delete.
(enabled): Remove insn_enabled test.
(*arith_shiftsi): Split out...
(*arith_multsi): ...this pattern and remove insn_enabled attribute.
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md (revision 210972)
+++ gcc/config/arm/arm.md (working copy)
@@ -200,17 +200,9 @@
(const_string "yes")]
(const_string "no")))
-; Allows an insn to disable certain alternatives for reasons other than
-; arch support.
-(define_attr "insn_enabled" "no,yes"
- (const_string "yes"))
-
; Enable all alternatives that are both arch_enabled and insn_enabled.
(define_attr "enabled" "no,yes"
- (cond [(eq_attr "insn_enabled" "no")
- (const_string "no")
-
- (and (eq_attr "predicable_short_it" "no")
+ (cond [(and (eq_attr "predicable_short_it" "no")
(and (eq_attr "predicated" "yes")
(match_test "arm_restrict_it")))
(const_string "no")
@@ -9876,39 +9868,38 @@
\f
;; Patterns to allow combination of arithmetic, cond code and shifts
-(define_insn "*arith_shiftsi"
- [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
- (match_operator:SI 1 "shiftable_operator"
- [(match_operator:SI 3 "shift_operator"
- [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
- (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
- (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
+;; Separate out the (mult ...) case, since it doesn't allow register operands.
+(define_insn "*arith_multsi"
+ [(set (match_operand:SI 0 "s_register_operand" "=r,r")
+ (shiftable_ops:SI
+ (match_operator:SI 2 "mult_operator"
+ [(match_operand:SI 3 "s_register_operand" "r,r")
+ (match_operand:SI 4 "power_of_two_operand")])
+ (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
"TARGET_32BIT"
- "%i1%?\\t%0, %2, %4%S3"
+ "<arith_shift_insn>%?\\t%0, %1, %3%S2"
[(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "no")
(set_attr "shift" "4")
- (set_attr "arch" "a,t2,t2,a")
- ;; Thumb2 doesn't allow the stack pointer to be used for
- ;; operand1 for all operations other than add and sub. In this case
- ;; the minus operation is a candidate for an rsub and hence needs
- ;; to be disabled.
- ;; We have to make sure to disable the fourth alternative if
- ;; the shift_operator is MULT, since otherwise the insn will
- ;; also match a multiply_accumulate pattern and validate_change
- ;; will allow a replacement of the constant with a register
- ;; despite the checks done in shift_operator.
- (set_attr_alternative "insn_enabled"
- [(const_string "yes")
- (if_then_else
- (match_operand:SI 1 "add_operator" "")
- (const_string "yes") (const_string "no"))
- (const_string "yes")
- (if_then_else
- (match_operand:SI 3 "mult_operator" "")
- (const_string "no") (const_string "yes"))])
- (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
+ (set_attr "arch" "a,t2")
+ (set_attr "type" "alu_shift_imm,alu_shift_imm")])
+;; Handles cases other than the above.
+(define_insn "*arith_shiftsi"
+ [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
+ (shiftable_ops:SI
+ (match_operator:SI 2 "shift_operator"
+ [(match_operand:SI 3 "s_register_operand" "r,r,r")
+ (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
+ (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
+ "TARGET_32BIT && GET_CODE (operands[3]) != MULT"
+ "<arith_shift_insn>%?\\t%0, %1, %3%S2"
+ [(set_attr "predicable" "yes")
+ (set_attr "predicable_short_it" "no")
+ (set_attr "shift" "4")
+ (set_attr "arch" "a,t2,a")
+ (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
+
(define_split
[(set (match_operand:SI 0 "s_register_operand" "")
(match_operator:SI 1 "shiftable_operator"
Index: gcc/config/arm/iterators.md
===================================================================
--- gcc/config/arm/iterators.md (revision 210972)
+++ gcc/config/arm/iterators.md (working copy)
@@ -194,6 +194,20 @@
;; Right shifts
(define_code_iterator rshifts [ashiftrt lshiftrt])
+;; Binary operators whose second operand can be shifted.
+(define_code_iterator shiftable_ops [plus minus ior xor and])
+
+;; plus and minus are the only shiftable_ops for which Thumb2 allows
+;; a stack pointer opoerand. The minus operation is a candidate for an rsub
+;; and hence only plus is supported.
+(define_code_attr t2_binop0
+ [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
+
+;; The instruction to use when a shiftable_ops has a shift operation as
+;; its first operand.
+(define_code_attr arith_shift_insn
+ [(plus "add") (minus "rsb") (ior "orr") (xor "eor") (and "and")])
+
;;----------------------------------------------------------------------------
;; Int iterators
;;----------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 14:08 ` Richard Sandiford
2014-05-27 14:18 ` Richard Sandiford
@ 2014-05-27 15:15 ` Richard Earnshaw
2014-05-27 15:27 ` Jakub Jelinek
1 sibling, 1 reply; 25+ messages in thread
From: Richard Earnshaw @ 2014-05-27 15:15 UTC (permalink / raw)
To: Christophe Lyon, Jeff Law, gcc-patches, rsandifo
On 27/05/14 15:08, Richard Sandiford wrote:
> Hmm, is this because of "insn_enabled"? If so, how did that work before
> the patch? LRA already assumed that the "enabled" attribute didn't depend
> on the operands.
Huh! "enabled" can be applied to each alternative. Alternatives are
selected based on the operands. If LRA can't cope with that we have a
serious problem. In fact, a pattern that matches but has no enabled
alternatives is meaningless and guaranteed to cause problems during
register allocation.
R.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:15 ` Richard Earnshaw
@ 2014-05-27 15:27 ` Jakub Jelinek
2014-05-27 15:40 ` Richard Earnshaw
0 siblings, 1 reply; 25+ messages in thread
From: Jakub Jelinek @ 2014-05-27 15:27 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: Christophe Lyon, Jeff Law, gcc-patches, rsandifo
On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
> On 27/05/14 15:08, Richard Sandiford wrote:
> > Hmm, is this because of "insn_enabled"? If so, how did that work before
> > the patch? LRA already assumed that the "enabled" attribute didn't depend
> > on the operands.
>
> Huh! "enabled" can be applied to each alternative. Alternatives are
> selected based on the operands. If LRA can't cope with that we have a
> serious problem. In fact, a pattern that matches but has no enabled
> alternatives is meaningless and guaranteed to cause problems during
> register allocation.
This is not LRA fault, but the backend misusing the "enabled" attribute
for something it wasn't designed for, and IMHO against the documentation
of the attribute too.
Just look at the original submission why it has been added.
Jakub
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:27 ` Jakub Jelinek
@ 2014-05-27 15:40 ` Richard Earnshaw
2014-05-27 15:50 ` Jakub Jelinek
2014-05-27 16:09 ` Richard Sandiford
0 siblings, 2 replies; 25+ messages in thread
From: Richard Earnshaw @ 2014-05-27 15:40 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Christophe Lyon, Jeff Law, gcc-patches, rsandifo
On 27/05/14 16:27, Jakub Jelinek wrote:
> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>> On 27/05/14 15:08, Richard Sandiford wrote:
>>> Hmm, is this because of "insn_enabled"? If so, how did that work before
>>> the patch? LRA already assumed that the "enabled" attribute didn't depend
>>> on the operands.
>>
>> Huh! "enabled" can be applied to each alternative. Alternatives are
>> selected based on the operands. If LRA can't cope with that we have a
>> serious problem. In fact, a pattern that matches but has no enabled
>> alternatives is meaningless and guaranteed to cause problems during
>> register allocation.
>
> This is not LRA fault, but the backend misusing the "enabled" attribute
> for something it wasn't designed for, and IMHO against the documentation
> of the attribute too.
> Just look at the original submission why it has been added.
>
> Jakub
>
<quote>
The @code{enabled} insn attribute may be used to disable certain insn
alternatives for machine-specific reasons.
<quote>
The rest of the text just says what happens when this is done and then
gives an example usage. It doesn't any time, either explicitly or
implicitly, say that this must be a static choice determined once-off at
run time.
That being said, I agree that this particular use case is pushing the
boundaries -- but that's always a risk when the boundaries aren't
clearly defined.
A better solution here would be to get rid of all those match_operator
patterns and replace them with iterators; but that's a lot of work,
particularly for all the conditinal operation patterns we have. It
would probably also bloat the number of patterns quite alarmingly.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:40 ` Richard Earnshaw
@ 2014-05-27 15:50 ` Jakub Jelinek
2014-05-27 16:01 ` Richard Earnshaw
2014-05-27 16:09 ` Richard Sandiford
1 sibling, 1 reply; 25+ messages in thread
From: Jakub Jelinek @ 2014-05-27 15:50 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: Christophe Lyon, Jeff Law, gcc-patches, rsandifo
On Tue, May 27, 2014 at 04:40:13PM +0100, Richard Earnshaw wrote:
> <quote>
> The @code{enabled} insn attribute may be used to disable certain insn
> alternatives for machine-specific reasons.
> <quote>
>
> The rest of the text just says what happens when this is done and then
> gives an example usage. It doesn't any time, either explicitly or
> implicitly, say that this must be a static choice determined once-off at
> run time.
It says:
"This definition should be based on other insn attributes and/or target flags."
where from what I understand Andreas originally meant was that the other
insn attributes would be again based on target flags or other such insn
attributes. Yeah, it explicitly rules out just basing the enabled attribute on
the operands, arm uses just another attribute based on the operands there,
but it still goes against the intent of the attribute, and is definitely a
nightmare for the register allocator.
Decisions based on the operands should be through constraints.
From what I can see, arm uses it just on a single insn, so I'd say it
shouldn't be a big deal to rewrite it.
Jakub
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:50 ` Jakub Jelinek
@ 2014-05-27 16:01 ` Richard Earnshaw
0 siblings, 0 replies; 25+ messages in thread
From: Richard Earnshaw @ 2014-05-27 16:01 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Christophe Lyon, Jeff Law, gcc-patches, rsandifo
On 27/05/14 16:50, Jakub Jelinek wrote:
> On Tue, May 27, 2014 at 04:40:13PM +0100, Richard Earnshaw wrote:
>> <quote>
>> The @code{enabled} insn attribute may be used to disable certain insn
>> alternatives for machine-specific reasons.
>> <quote>
>>
>> The rest of the text just says what happens when this is done and then
>> gives an example usage. It doesn't any time, either explicitly or
>> implicitly, say that this must be a static choice determined once-off at
>> run time.
>
> It says:
> "This definition should be based on other insn attributes and/or target flags."
<pedantry>
- It only says *should*, not *must*. "Should" can imply a
recommendation, not a requirement.
- It only says *based on*, not *solely based on*. Based on implies that
other information might be involved as well, without limitation.
</pedantary>
> where from what I understand Andreas originally meant was that the other
> insn attributes would be again based on target flags or other such insn
> attributes. Yeah, it explicitly rules out just basing the enabled attribute on
> the operands, arm uses just another attribute based on the operands there,
> but it still goes against the intent of the attribute, and is definitely a
> nightmare for the register allocator.
> Decisions based on the operands should be through constraints.
>
> From what I can see, arm uses it just on a single insn, so I'd say it
> shouldn't be a big deal to rewrite it.
>
Pedantry aside, I'm not against making such a restriction; but it is a
new restriction that isn't explictly stated in the documentation today.
R.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:40 ` Richard Earnshaw
2014-05-27 15:50 ` Jakub Jelinek
@ 2014-05-27 16:09 ` Richard Sandiford
2014-05-27 16:21 ` Richard Earnshaw
1 sibling, 1 reply; 25+ messages in thread
From: Richard Sandiford @ 2014-05-27 16:09 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: Jakub Jelinek, Christophe Lyon, Jeff Law, gcc-patches
Richard Earnshaw <rearnsha@arm.com> writes:
> On 27/05/14 16:27, Jakub Jelinek wrote:
>> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>>> On 27/05/14 15:08, Richard Sandiford wrote:
>>>> Hmm, is this because of "insn_enabled"? If so, how did that work before
>>>> the patch? LRA already assumed that the "enabled" attribute didn't depend
>>>> on the operands.
>>>
>>> Huh! "enabled" can be applied to each alternative. Alternatives are
>>> selected based on the operands. If LRA can't cope with that we have a
>>> serious problem. In fact, a pattern that matches but has no enabled
>>> alternatives is meaningless and guaranteed to cause problems during
>>> register allocation.
>>
>> This is not LRA fault, but the backend misusing the "enabled" attribute
>> for something it wasn't designed for, and IMHO against the documentation
>> of the attribute too.
>> Just look at the original submission why it has been added.
>>
>> Jakub
>>
>
> <quote>
> The @code{enabled} insn attribute may be used to disable certain insn
> alternatives for machine-specific reasons.
> <quote>
>
> The rest of the text just says what happens when this is done and then
> gives an example usage. It doesn't any time, either explicitly or
> implicitly, say that this must be a static choice determined once-off at
> run time.
OK, how about the doc patch below?
> That being said, I agree that this particular use case is pushing the
> boundaries -- but that's always a risk when the boundaries aren't
> clearly defined.
>
> A better solution here would be to get rid of all those match_operator
> patterns and replace them with iterators; but that's a lot of work,
> particularly for all the conditinal operation patterns we have. It
> would probably also bloat the number of patterns quite alarmingly.
Yeah, which is why I was just going for the one place where it mattered.
I think match_operator still has a place in cases where the insn pattern
is entirely regular, which seems to be the case for most other uses
of shiftable_operator. It's just that in this case there really were
two separate cases per operator (plus vs. non-plus and mult vs. true shift).
Thanks,
Richard
gcc/
* doc/md.texi: Document the restrictions on the "enabled" attribute.
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi (revision 210972)
+++ gcc/doc/md.texi (working copy)
@@ -4094,11 +4094,11 @@
@subsection Disable insn alternatives using the @code{enabled} attribute
@cindex enabled
-The @code{enabled} insn attribute may be used to disable certain insn
-alternatives for machine-specific reasons. This is useful when adding
-new instructions to an existing pattern which are only available for
-certain cpu architecture levels as specified with the @code{-march=}
-option.
+The @code{enabled} insn attribute may be used to disable insn
+alternatives that are not available for the current subtarget.
+This is useful when adding new instructions to an existing pattern
+which are only available for certain cpu architecture levels as
+specified with the @code{-march=} option.
If an insn alternative is disabled, then it will never be used. The
compiler treats the constraints for the disabled alternative as
@@ -4112,6 +4112,9 @@
A definition of the @code{enabled} insn attribute. The attribute is
defined as usual using the @code{define_attr} command. This
definition should be based on other insn attributes and/or target flags.
+It must not depend directly or indirectly on the current operands,
+since the attribute is expected to be a static property of the subtarget.
+
The @code{enabled} attribute is a numeric attribute and should evaluate to
@code{(const_int 1)} for an enabled alternative and to
@code{(const_int 0)} otherwise.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 16:09 ` Richard Sandiford
@ 2014-05-27 16:21 ` Richard Earnshaw
2014-05-27 16:31 ` Richard Sandiford
0 siblings, 1 reply; 25+ messages in thread
From: Richard Earnshaw @ 2014-05-27 16:21 UTC (permalink / raw)
To: Richard Earnshaw, Jakub Jelinek, Christophe Lyon, Jeff Law,
gcc-patches, rdsandiford
On 27/05/14 17:09, Richard Sandiford wrote:
> Richard Earnshaw <rearnsha@arm.com> writes:
>> On 27/05/14 16:27, Jakub Jelinek wrote:
>>> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>>>> On 27/05/14 15:08, Richard Sandiford wrote:
>>>>> Hmm, is this because of "insn_enabled"? If so, how did that work before
>>>>> the patch? LRA already assumed that the "enabled" attribute didn't depend
>>>>> on the operands.
>>>>
>>>> Huh! "enabled" can be applied to each alternative. Alternatives are
>>>> selected based on the operands. If LRA can't cope with that we have a
>>>> serious problem. In fact, a pattern that matches but has no enabled
>>>> alternatives is meaningless and guaranteed to cause problems during
>>>> register allocation.
>>>
>>> This is not LRA fault, but the backend misusing the "enabled" attribute
>>> for something it wasn't designed for, and IMHO against the documentation
>>> of the attribute too.
>>> Just look at the original submission why it has been added.
>>>
>>> Jakub
>>>
>>
>> <quote>
>> The @code{enabled} insn attribute may be used to disable certain insn
>> alternatives for machine-specific reasons.
>> <quote>
>>
>> The rest of the text just says what happens when this is done and then
>> gives an example usage. It doesn't any time, either explicitly or
>> implicitly, say that this must be a static choice determined once-off at
>> run time.
>
> OK, how about the doc patch below?
>
>> That being said, I agree that this particular use case is pushing the
>> boundaries -- but that's always a risk when the boundaries aren't
>> clearly defined.
>>
>> A better solution here would be to get rid of all those match_operator
>> patterns and replace them with iterators; but that's a lot of work,
>> particularly for all the conditinal operation patterns we have. It
>> would probably also bloat the number of patterns quite alarmingly.
>
> Yeah, which is why I was just going for the one place where it mattered.
> I think match_operator still has a place in cases where the insn pattern
> is entirely regular, which seems to be the case for most other uses
> of shiftable_operator. It's just that in this case there really were
> two separate cases per operator (plus vs. non-plus and mult vs. true shift).
>
> Thanks,
> Richard
>
>
> gcc/
> * doc/md.texi: Document the restrictions on the "enabled" attribute.
>
> Index: gcc/doc/md.texi
> ===================================================================
> --- gcc/doc/md.texi (revision 210972)
> +++ gcc/doc/md.texi (working copy)
> @@ -4094,11 +4094,11 @@
> @subsection Disable insn alternatives using the @code{enabled} attribute
> @cindex enabled
>
> -The @code{enabled} insn attribute may be used to disable certain insn
> -alternatives for machine-specific reasons. This is useful when adding
> -new instructions to an existing pattern which are only available for
> -certain cpu architecture levels as specified with the @code{-march=}
> -option.
> +The @code{enabled} insn attribute may be used to disable insn
> +alternatives that are not available for the current subtarget.
> +This is useful when adding new instructions to an existing pattern
> +which are only available for certain cpu architecture levels as
> +specified with the @code{-march=} option.
>
> If an insn alternative is disabled, then it will never be used. The
> compiler treats the constraints for the disabled alternative as
> @@ -4112,6 +4112,9 @@
> A definition of the @code{enabled} insn attribute. The attribute is
> defined as usual using the @code{define_attr} command. This
> definition should be based on other insn attributes and/or target flags.
> +It must not depend directly or indirectly on the current operands,
> +since the attribute is expected to be a static property of the subtarget.
> +
I'd reverse the two components of that sentence. Something like:
The attribute must be a static property of the subtarget; that is, it
must not depend on the current operands or any other dynamic context
(for example, location of the insn within the body of a loop).
It would be useful if we could precisely define 'static property'
somewhere, so that it encompases per-function changing of the ISA or
optimization variant via __attribute__ annotations. That does need to
work, since it could be used for switching between ARM and Thumb.
R.
> The @code{enabled} attribute is a numeric attribute and should evaluate to
> @code{(const_int 1)} for an enabled alternative and to
> @code{(const_int 0)} otherwise.
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 16:21 ` Richard Earnshaw
@ 2014-05-27 16:31 ` Richard Sandiford
2014-05-28 15:11 ` Richard Earnshaw
0 siblings, 1 reply; 25+ messages in thread
From: Richard Sandiford @ 2014-05-27 16:31 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: Jakub Jelinek, Christophe Lyon, Jeff Law, gcc-patches
Richard Earnshaw <rearnsha@arm.com> writes:
> On 27/05/14 17:09, Richard Sandiford wrote:
>> Richard Earnshaw <rearnsha@arm.com> writes:
>>> On 27/05/14 16:27, Jakub Jelinek wrote:
>>>> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>>>>> On 27/05/14 15:08, Richard Sandiford wrote:
>>>>>> Hmm, is this because of "insn_enabled"? If so, how did that work before
>>>>>> the patch? LRA already assumed that the "enabled" attribute didn't depend
>>>>>> on the operands.
>>>>>
>>>>> Huh! "enabled" can be applied to each alternative. Alternatives are
>>>>> selected based on the operands. If LRA can't cope with that we have a
>>>>> serious problem. In fact, a pattern that matches but has no enabled
>>>>> alternatives is meaningless and guaranteed to cause problems during
>>>>> register allocation.
>>>>
>>>> This is not LRA fault, but the backend misusing the "enabled" attribute
>>>> for something it wasn't designed for, and IMHO against the documentation
>>>> of the attribute too.
>>>> Just look at the original submission why it has been added.
>>>>
>>>> Jakub
>>>>
>>>
>>> <quote>
>>> The @code{enabled} insn attribute may be used to disable certain insn
>>> alternatives for machine-specific reasons.
>>> <quote>
>>>
>>> The rest of the text just says what happens when this is done and then
>>> gives an example usage. It doesn't any time, either explicitly or
>>> implicitly, say that this must be a static choice determined once-off at
>>> run time.
>>
>> OK, how about the doc patch below?
>>
>>> That being said, I agree that this particular use case is pushing the
>>> boundaries -- but that's always a risk when the boundaries aren't
>>> clearly defined.
>>>
>>> A better solution here would be to get rid of all those match_operator
>>> patterns and replace them with iterators; but that's a lot of work,
>>> particularly for all the conditinal operation patterns we have. It
>>> would probably also bloat the number of patterns quite alarmingly.
>>
>> Yeah, which is why I was just going for the one place where it mattered.
>> I think match_operator still has a place in cases where the insn pattern
>> is entirely regular, which seems to be the case for most other uses
>> of shiftable_operator. It's just that in this case there really were
>> two separate cases per operator (plus vs. non-plus and mult vs. true shift).
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> * doc/md.texi: Document the restrictions on the "enabled" attribute.
>>
>> Index: gcc/doc/md.texi
>> ===================================================================
>> --- gcc/doc/md.texi (revision 210972)
>> +++ gcc/doc/md.texi (working copy)
>> @@ -4094,11 +4094,11 @@
>> @subsection Disable insn alternatives using the @code{enabled} attribute
>> @cindex enabled
>>
>> -The @code{enabled} insn attribute may be used to disable certain insn
>> -alternatives for machine-specific reasons. This is useful when adding
>> -new instructions to an existing pattern which are only available for
>> -certain cpu architecture levels as specified with the @code{-march=}
>> -option.
>> +The @code{enabled} insn attribute may be used to disable insn
>> +alternatives that are not available for the current subtarget.
>> +This is useful when adding new instructions to an existing pattern
>> +which are only available for certain cpu architecture levels as
>> +specified with the @code{-march=} option.
>>
>> If an insn alternative is disabled, then it will never be used. The
>> compiler treats the constraints for the disabled alternative as
>> @@ -4112,6 +4112,9 @@
>> A definition of the @code{enabled} insn attribute. The attribute is
>> defined as usual using the @code{define_attr} command. This
>> definition should be based on other insn attributes and/or target flags.
>> +It must not depend directly or indirectly on the current operands,
>> +since the attribute is expected to be a static property of the subtarget.
>> +
>
> I'd reverse the two components of that sentence. Something like:
>
> The attribute must be a static property of the subtarget; that is, it
> must not depend on the current operands or any other dynamic context
> (for example, location of the insn within the body of a loop).
OK, how about the attached?
> It would be useful if we could precisely define 'static property'
> somewhere, so that it encompases per-function changing of the ISA or
> optimization variant via __attribute__ annotations. That does need to
> work, since it could be used for switching between ARM and Thumb.
Yeah, the cache depends on the current target for SWITCHABLE_TARGETs,
is invalidated by target_reinit for other targets, and is also invalidated
by changes to the register classes.
Thanks,
Richard
gcc/
* doc/md.texi: Document the restrictions on the "enabled" attribute.
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi (revision 210972)
+++ gcc/doc/md.texi (working copy)
@@ -4094,11 +4094,11 @@
@subsection Disable insn alternatives using the @code{enabled} attribute
@cindex enabled
-The @code{enabled} insn attribute may be used to disable certain insn
-alternatives for machine-specific reasons. This is useful when adding
-new instructions to an existing pattern which are only available for
-certain cpu architecture levels as specified with the @code{-march=}
-option.
+The @code{enabled} insn attribute may be used to disable insn
+alternatives that are not available for the current subtarget.
+This is useful when adding new instructions to an existing pattern
+which are only available for certain cpu architecture levels as
+specified with the @code{-march=} option.
If an insn alternative is disabled, then it will never be used. The
compiler treats the constraints for the disabled alternative as
@@ -4112,6 +4112,10 @@
A definition of the @code{enabled} insn attribute. The attribute is
defined as usual using the @code{define_attr} command. This
definition should be based on other insn attributes and/or target flags.
+The attribute must be a static property of the subtarget; that is, it
+must not depend on the current operands or any other dynamic context
+(for example, the location of the insn within the body of a loop).
+
The @code{enabled} attribute is a numeric attribute and should evaluate to
@code{(const_int 1)} for an enabled alternative and to
@code{(const_int 0)} otherwise.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:07 ` Richard Sandiford
@ 2014-05-27 16:36 ` Christophe Lyon
2014-05-28 14:01 ` Yufeng Zhang
2014-05-29 9:43 ` Richard Earnshaw
2 siblings, 0 replies; 25+ messages in thread
From: Christophe Lyon @ 2014-05-27 16:36 UTC (permalink / raw)
To: Christophe Lyon, Jeff Law, gcc-patches, Richard Sandiford
On 27 May 2014 17:07, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
>>> Does the following patch help?
>>
>> Bah, it won't of course: %i1 needs to be the operator.
>
> Here's v2. I tested that it worked for simple tests like:
>
I confirm that the compiler now builds fine (target
arm-none-linux-gnueabihf) with this patch applied.
Thanks,
Christophe.
> int f1 (int x, int y) { return x + (y << 4); }
> int f2 (int x, int y) { return x - (y << 4); }
> int f3 (int x, int y) { return x & (y << 4); }
> int f4 (int x, int y) { return x | (y << 4); }
> int f5 (int x, int y) { return x ^ (y << 4); }
> int f6 (int x, int y) { return (y << 4) - x; }
> int g1 (int x, int y, int z) { return x + (y << z); }
> int g2 (int x, int y, int z) { return x - (y << z); }
> int g3 (int x, int y, int z) { return x & (y << z); }
> int g4 (int x, int y, int z) { return x | (y << z); }
> int g5 (int x, int y, int z) { return x ^ (y << z); }
> int g6 (int x, int y, int z) { return (y << z) - x; }
>
> as well as the testcase.
>
> Thanks,
> Richard
>
>
> gcc/
> * config/arm/iterators.md (shiftable_ops): New code iterator.
> (t2_binop0, arith_shift_insn): New code attributes.
> * config/arm/arm.md (insn_enabled): Delete.
> (enabled): Remove insn_enabled test.
> (*arith_shiftsi): Split out...
> (*arith_multsi): ...this pattern and remove insn_enabled attribute.
>
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md (revision 210972)
> +++ gcc/config/arm/arm.md (working copy)
> @@ -200,17 +200,9 @@
> (const_string "yes")]
> (const_string "no")))
>
> -; Allows an insn to disable certain alternatives for reasons other than
> -; arch support.
> -(define_attr "insn_enabled" "no,yes"
> - (const_string "yes"))
> -
> ; Enable all alternatives that are both arch_enabled and insn_enabled.
> (define_attr "enabled" "no,yes"
> - (cond [(eq_attr "insn_enabled" "no")
> - (const_string "no")
> -
> - (and (eq_attr "predicable_short_it" "no")
> + (cond [(and (eq_attr "predicable_short_it" "no")
> (and (eq_attr "predicated" "yes")
> (match_test "arm_restrict_it")))
> (const_string "no")
> @@ -9876,39 +9868,38 @@
>
> ;; Patterns to allow combination of arithmetic, cond code and shifts
>
> -(define_insn "*arith_shiftsi"
> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
> - (match_operator:SI 1 "shiftable_operator"
> - [(match_operator:SI 3 "shift_operator"
> - [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
> - (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
> - (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
> +;; Separate out the (mult ...) case, since it doesn't allow register operands.
> +(define_insn "*arith_multsi"
> + [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> + (shiftable_ops:SI
> + (match_operator:SI 2 "mult_operator"
> + [(match_operand:SI 3 "s_register_operand" "r,r")
> + (match_operand:SI 4 "power_of_two_operand")])
> + (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
> "TARGET_32BIT"
> - "%i1%?\\t%0, %2, %4%S3"
> + "<arith_shift_insn>%?\\t%0, %1, %3%S2"
> [(set_attr "predicable" "yes")
> (set_attr "predicable_short_it" "no")
> (set_attr "shift" "4")
> - (set_attr "arch" "a,t2,t2,a")
> - ;; Thumb2 doesn't allow the stack pointer to be used for
> - ;; operand1 for all operations other than add and sub. In this case
> - ;; the minus operation is a candidate for an rsub and hence needs
> - ;; to be disabled.
> - ;; We have to make sure to disable the fourth alternative if
> - ;; the shift_operator is MULT, since otherwise the insn will
> - ;; also match a multiply_accumulate pattern and validate_change
> - ;; will allow a replacement of the constant with a register
> - ;; despite the checks done in shift_operator.
> - (set_attr_alternative "insn_enabled"
> - [(const_string "yes")
> - (if_then_else
> - (match_operand:SI 1 "add_operator" "")
> - (const_string "yes") (const_string "no"))
> - (const_string "yes")
> - (if_then_else
> - (match_operand:SI 3 "mult_operator" "")
> - (const_string "no") (const_string "yes"))])
> - (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> + (set_attr "arch" "a,t2")
> + (set_attr "type" "alu_shift_imm,alu_shift_imm")])
>
> +;; Handles cases other than the above.
> +(define_insn "*arith_shiftsi"
> + [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> + (shiftable_ops:SI
> + (match_operator:SI 2 "shift_operator"
> + [(match_operand:SI 3 "s_register_operand" "r,r,r")
> + (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
> + (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
> + "TARGET_32BIT && GET_CODE (operands[3]) != MULT"
> + "<arith_shift_insn>%?\\t%0, %1, %3%S2"
> + [(set_attr "predicable" "yes")
> + (set_attr "predicable_short_it" "no")
> + (set_attr "shift" "4")
> + (set_attr "arch" "a,t2,a")
> + (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> +
> (define_split
> [(set (match_operand:SI 0 "s_register_operand" "")
> (match_operator:SI 1 "shiftable_operator"
> Index: gcc/config/arm/iterators.md
> ===================================================================
> --- gcc/config/arm/iterators.md (revision 210972)
> +++ gcc/config/arm/iterators.md (working copy)
> @@ -194,6 +194,20 @@
> ;; Right shifts
> (define_code_iterator rshifts [ashiftrt lshiftrt])
>
> +;; Binary operators whose second operand can be shifted.
> +(define_code_iterator shiftable_ops [plus minus ior xor and])
> +
> +;; plus and minus are the only shiftable_ops for which Thumb2 allows
> +;; a stack pointer opoerand. The minus operation is a candidate for an rsub
> +;; and hence only plus is supported.
> +(define_code_attr t2_binop0
> + [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
> +
> +;; The instruction to use when a shiftable_ops has a shift operation as
> +;; its first operand.
> +(define_code_attr arith_shift_insn
> + [(plus "add") (minus "rsb") (ior "orr") (xor "eor") (and "and")])
> +
> ;;----------------------------------------------------------------------------
> ;; Int iterators
> ;;----------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:07 ` Richard Sandiford
2014-05-27 16:36 ` Christophe Lyon
@ 2014-05-28 14:01 ` Yufeng Zhang
2014-05-29 9:43 ` Richard Earnshaw
2 siblings, 0 replies; 25+ messages in thread
From: Yufeng Zhang @ 2014-05-28 14:01 UTC (permalink / raw)
To: gcc-patches, rdsandiford; +Cc: Christophe Lyon, Jeff Law
The patch also fixes the arm-none-eabi build failures I've seen.
Thanks,
Yufeng
On 05/27/14 16:07, Richard Sandiford wrote:
> Richard Sandiford<rdsandiford@googlemail.com> writes:
>> Richard Sandiford<rsandifo@linux.vnet.ibm.com> writes:
>>> Does the following patch help?
>>
>> Bah, it won't of course: %i1 needs to be the operator.
>
> Here's v2. I tested that it worked for simple tests like:
>
> int f1 (int x, int y) { return x + (y<< 4); }
> int f2 (int x, int y) { return x - (y<< 4); }
> int f3 (int x, int y) { return x& (y<< 4); }
> int f4 (int x, int y) { return x | (y<< 4); }
> int f5 (int x, int y) { return x ^ (y<< 4); }
> int f6 (int x, int y) { return (y<< 4) - x; }
> int g1 (int x, int y, int z) { return x + (y<< z); }
> int g2 (int x, int y, int z) { return x - (y<< z); }
> int g3 (int x, int y, int z) { return x& (y<< z); }
> int g4 (int x, int y, int z) { return x | (y<< z); }
> int g5 (int x, int y, int z) { return x ^ (y<< z); }
> int g6 (int x, int y, int z) { return (y<< z) - x; }
>
> as well as the testcase.
>
> Thanks,
> Richard
>
>
> gcc/
> * config/arm/iterators.md (shiftable_ops): New code iterator.
> (t2_binop0, arith_shift_insn): New code attributes.
> * config/arm/arm.md (insn_enabled): Delete.
> (enabled): Remove insn_enabled test.
> (*arith_shiftsi): Split out...
> (*arith_multsi): ...this pattern and remove insn_enabled attribute.
>
> Index: gcc/config/arm/arm.md
> ===================================================================
> --- gcc/config/arm/arm.md (revision 210972)
> +++ gcc/config/arm/arm.md (working copy)
> @@ -200,17 +200,9 @@
> (const_string "yes")]
> (const_string "no")))
>
> -; Allows an insn to disable certain alternatives for reasons other than
> -; arch support.
> -(define_attr "insn_enabled" "no,yes"
> - (const_string "yes"))
> -
> ; Enable all alternatives that are both arch_enabled and insn_enabled.
> (define_attr "enabled" "no,yes"
> - (cond [(eq_attr "insn_enabled" "no")
> - (const_string "no")
> -
> - (and (eq_attr "predicable_short_it" "no")
> + (cond [(and (eq_attr "predicable_short_it" "no")
> (and (eq_attr "predicated" "yes")
> (match_test "arm_restrict_it")))
> (const_string "no")
> @@ -9876,39 +9868,38 @@
> \f
> ;; Patterns to allow combination of arithmetic, cond code and shifts
>
> -(define_insn "*arith_shiftsi"
> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
> - (match_operator:SI 1 "shiftable_operator"
> - [(match_operator:SI 3 "shift_operator"
> - [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
> - (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
> - (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
> +;; Separate out the (mult ...) case, since it doesn't allow register operands.
> +(define_insn "*arith_multsi"
> + [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> + (shiftable_ops:SI
> + (match_operator:SI 2 "mult_operator"
> + [(match_operand:SI 3 "s_register_operand" "r,r")
> + (match_operand:SI 4 "power_of_two_operand")])
> + (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
> "TARGET_32BIT"
> - "%i1%?\\t%0, %2, %4%S3"
> + "<arith_shift_insn>%?\\t%0, %1, %3%S2"
> [(set_attr "predicable" "yes")
> (set_attr "predicable_short_it" "no")
> (set_attr "shift" "4")
> - (set_attr "arch" "a,t2,t2,a")
> - ;; Thumb2 doesn't allow the stack pointer to be used for
> - ;; operand1 for all operations other than add and sub. In this case
> - ;; the minus operation is a candidate for an rsub and hence needs
> - ;; to be disabled.
> - ;; We have to make sure to disable the fourth alternative if
> - ;; the shift_operator is MULT, since otherwise the insn will
> - ;; also match a multiply_accumulate pattern and validate_change
> - ;; will allow a replacement of the constant with a register
> - ;; despite the checks done in shift_operator.
> - (set_attr_alternative "insn_enabled"
> - [(const_string "yes")
> - (if_then_else
> - (match_operand:SI 1 "add_operator" "")
> - (const_string "yes") (const_string "no"))
> - (const_string "yes")
> - (if_then_else
> - (match_operand:SI 3 "mult_operator" "")
> - (const_string "no") (const_string "yes"))])
> - (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> + (set_attr "arch" "a,t2")
> + (set_attr "type" "alu_shift_imm,alu_shift_imm")])
>
> +;; Handles cases other than the above.
> +(define_insn "*arith_shiftsi"
> + [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> + (shiftable_ops:SI
> + (match_operator:SI 2 "shift_operator"
> + [(match_operand:SI 3 "s_register_operand" "r,r,r")
> + (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
> + (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
> + "TARGET_32BIT&& GET_CODE (operands[3]) != MULT"
> + "<arith_shift_insn>%?\\t%0, %1, %3%S2"
> + [(set_attr "predicable" "yes")
> + (set_attr "predicable_short_it" "no")
> + (set_attr "shift" "4")
> + (set_attr "arch" "a,t2,a")
> + (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
> +
> (define_split
> [(set (match_operand:SI 0 "s_register_operand" "")
> (match_operator:SI 1 "shiftable_operator"
> Index: gcc/config/arm/iterators.md
> ===================================================================
> --- gcc/config/arm/iterators.md (revision 210972)
> +++ gcc/config/arm/iterators.md (working copy)
> @@ -194,6 +194,20 @@
> ;; Right shifts
> (define_code_iterator rshifts [ashiftrt lshiftrt])
>
> +;; Binary operators whose second operand can be shifted.
> +(define_code_iterator shiftable_ops [plus minus ior xor and])
> +
> +;; plus and minus are the only shiftable_ops for which Thumb2 allows
> +;; a stack pointer opoerand. The minus operation is a candidate for an rsub
> +;; and hence only plus is supported.
> +(define_code_attr t2_binop0
> + [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
> +
> +;; The instruction to use when a shiftable_ops has a shift operation as
> +;; its first operand.
> +(define_code_attr arith_shift_insn
> + [(plus "add") (minus "rsb") (ior "orr") (xor "eor") (and "and")])
> +
> ;;----------------------------------------------------------------------------
> ;; Int iterators
> ;;----------------------------------------------------------------------------
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 16:31 ` Richard Sandiford
@ 2014-05-28 15:11 ` Richard Earnshaw
0 siblings, 0 replies; 25+ messages in thread
From: Richard Earnshaw @ 2014-05-28 15:11 UTC (permalink / raw)
To: Richard Earnshaw, Jakub Jelinek, Christophe Lyon, Jeff Law,
gcc-patches, rdsandiford
On 27/05/14 17:31, Richard Sandiford wrote:
> Richard Earnshaw <rearnsha@arm.com> writes:
>> On 27/05/14 17:09, Richard Sandiford wrote:
>>> Richard Earnshaw <rearnsha@arm.com> writes:
>>>> On 27/05/14 16:27, Jakub Jelinek wrote:
>>>>> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>>>>>> On 27/05/14 15:08, Richard Sandiford wrote:
>>>>>>> Hmm, is this because of "insn_enabled"? If so, how did that work before
>>>>>>> the patch? LRA already assumed that the "enabled" attribute didn't depend
>>>>>>> on the operands.
>>>>>>
>>>>>> Huh! "enabled" can be applied to each alternative. Alternatives are
>>>>>> selected based on the operands. If LRA can't cope with that we have a
>>>>>> serious problem. In fact, a pattern that matches but has no enabled
>>>>>> alternatives is meaningless and guaranteed to cause problems during
>>>>>> register allocation.
>>>>>
>>>>> This is not LRA fault, but the backend misusing the "enabled" attribute
>>>>> for something it wasn't designed for, and IMHO against the documentation
>>>>> of the attribute too.
>>>>> Just look at the original submission why it has been added.
>>>>>
>>>>> Jakub
>>>>>
>>>>
>>>> <quote>
>>>> The @code{enabled} insn attribute may be used to disable certain insn
>>>> alternatives for machine-specific reasons.
>>>> <quote>
>>>>
>>>> The rest of the text just says what happens when this is done and then
>>>> gives an example usage. It doesn't any time, either explicitly or
>>>> implicitly, say that this must be a static choice determined once-off at
>>>> run time.
>>>
>>> OK, how about the doc patch below?
>>>
>>>> That being said, I agree that this particular use case is pushing the
>>>> boundaries -- but that's always a risk when the boundaries aren't
>>>> clearly defined.
>>>>
>>>> A better solution here would be to get rid of all those match_operator
>>>> patterns and replace them with iterators; but that's a lot of work,
>>>> particularly for all the conditinal operation patterns we have. It
>>>> would probably also bloat the number of patterns quite alarmingly.
>>>
>>> Yeah, which is why I was just going for the one place where it mattered.
>>> I think match_operator still has a place in cases where the insn pattern
>>> is entirely regular, which seems to be the case for most other uses
>>> of shiftable_operator. It's just that in this case there really were
>>> two separate cases per operator (plus vs. non-plus and mult vs. true shift).
>>>
>>> Thanks,
>>> Richard
>>>
>>>
>>> gcc/
>>> * doc/md.texi: Document the restrictions on the "enabled" attribute.
>>>
>>> Index: gcc/doc/md.texi
>>> ===================================================================
>>> --- gcc/doc/md.texi (revision 210972)
>>> +++ gcc/doc/md.texi (working copy)
>>> @@ -4094,11 +4094,11 @@
>>> @subsection Disable insn alternatives using the @code{enabled} attribute
>>> @cindex enabled
>>>
>>> -The @code{enabled} insn attribute may be used to disable certain insn
>>> -alternatives for machine-specific reasons. This is useful when adding
>>> -new instructions to an existing pattern which are only available for
>>> -certain cpu architecture levels as specified with the @code{-march=}
>>> -option.
>>> +The @code{enabled} insn attribute may be used to disable insn
>>> +alternatives that are not available for the current subtarget.
>>> +This is useful when adding new instructions to an existing pattern
>>> +which are only available for certain cpu architecture levels as
>>> +specified with the @code{-march=} option.
>>>
>>> If an insn alternative is disabled, then it will never be used. The
>>> compiler treats the constraints for the disabled alternative as
>>> @@ -4112,6 +4112,9 @@
>>> A definition of the @code{enabled} insn attribute. The attribute is
>>> defined as usual using the @code{define_attr} command. This
>>> definition should be based on other insn attributes and/or target flags.
>>> +It must not depend directly or indirectly on the current operands,
>>> +since the attribute is expected to be a static property of the subtarget.
>>> +
>>
>> I'd reverse the two components of that sentence. Something like:
>>
>> The attribute must be a static property of the subtarget; that is, it
>> must not depend on the current operands or any other dynamic context
>> (for example, location of the insn within the body of a loop).
>
> OK, how about the attached?
>
>> It would be useful if we could precisely define 'static property'
>> somewhere, so that it encompases per-function changing of the ISA or
>> optimization variant via __attribute__ annotations. That does need to
>> work, since it could be used for switching between ARM and Thumb.
>
> Yeah, the cache depends on the current target for SWITCHABLE_TARGETs,
> is invalidated by target_reinit for other targets, and is also invalidated
> by changes to the register classes.
>
> Thanks,
> Richard
>
>
> gcc/
> * doc/md.texi: Document the restrictions on the "enabled" attribute.
>
LGTM.
R.
> Index: gcc/doc/md.texi
> ===================================================================
> --- gcc/doc/md.texi (revision 210972)
> +++ gcc/doc/md.texi (working copy)
> @@ -4094,11 +4094,11 @@
> @subsection Disable insn alternatives using the @code{enabled} attribute
> @cindex enabled
>
> -The @code{enabled} insn attribute may be used to disable certain insn
> -alternatives for machine-specific reasons. This is useful when adding
> -new instructions to an existing pattern which are only available for
> -certain cpu architecture levels as specified with the @code{-march=}
> -option.
> +The @code{enabled} insn attribute may be used to disable insn
> +alternatives that are not available for the current subtarget.
> +This is useful when adding new instructions to an existing pattern
> +which are only available for certain cpu architecture levels as
> +specified with the @code{-march=} option.
>
> If an insn alternative is disabled, then it will never be used. The
> compiler treats the constraints for the disabled alternative as
> @@ -4112,6 +4112,10 @@
> A definition of the @code{enabled} insn attribute. The attribute is
> defined as usual using the @code{define_attr} command. This
> definition should be based on other insn attributes and/or target flags.
> +The attribute must be a static property of the subtarget; that is, it
> +must not depend on the current operands or any other dynamic context
> +(for example, the location of the insn within the body of a loop).
> +
> The @code{enabled} attribute is a numeric attribute and should evaluate to
> @code{(const_int 1)} for an enabled alternative and to
> @code{(const_int 0)} otherwise.
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-27 15:07 ` Richard Sandiford
2014-05-27 16:36 ` Christophe Lyon
2014-05-28 14:01 ` Yufeng Zhang
@ 2014-05-29 9:43 ` Richard Earnshaw
2014-05-29 10:45 ` Richard Sandiford
2 siblings, 1 reply; 25+ messages in thread
From: Richard Earnshaw @ 2014-05-29 9:43 UTC (permalink / raw)
To: Christophe Lyon, Jeff Law, gcc-patches, rdsandiford
[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]
On 27/05/14 16:07, Richard Sandiford wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
>>> Does the following patch help?
>>
>> Bah, it won't of course: %i1 needs to be the operator.
>
> Here's v2. I tested that it worked for simple tests like:
>
> int f1 (int x, int y) { return x + (y << 4); }
> int f2 (int x, int y) { return x - (y << 4); }
> int f3 (int x, int y) { return x & (y << 4); }
> int f4 (int x, int y) { return x | (y << 4); }
> int f5 (int x, int y) { return x ^ (y << 4); }
> int f6 (int x, int y) { return (y << 4) - x; }
> int g1 (int x, int y, int z) { return x + (y << z); }
> int g2 (int x, int y, int z) { return x - (y << z); }
> int g3 (int x, int y, int z) { return x & (y << z); }
> int g4 (int x, int y, int z) { return x | (y << z); }
> int g5 (int x, int y, int z) { return x ^ (y << z); }
> int g6 (int x, int y, int z) { return (y << z) - x; }
>
> as well as the testcase.
>
> Thanks,
> Richard
>
>
> gcc/
> * config/arm/iterators.md (shiftable_ops): New code iterator.
> (t2_binop0, arith_shift_insn): New code attributes.
> * config/arm/arm.md (insn_enabled): Delete.
> (enabled): Remove insn_enabled test.
> (*arith_shiftsi): Split out...
> (*arith_multsi): ...this pattern and remove insn_enabled attribute.
>
Thanks, Richard.
I've tweaked this as followed and committed it.
I now consider "shift_operator" in the arm backend deprecated. We
should be moving towards using shift_nomul_operator.
There's one final wart still to be handled, though. 'rotate' can only
take an immediate operand, not a register. We can currently deal with
this, but it's not clean in terms of constraint handling. I'll see if I
can fix this up sometime, but not today.
R.
2014-05-29 Richard Earnshaw <rearnsha@arm.com>
Richard Sandiford <rdsandiford@googlemail.com>
* arm/iterators.md (shiftable_ops): New code iterator.
(t2_binop0, arith_shift_insn): New code attributes.
* arm/predicates.md (shift_nomul_operator): New predicate.
* arm/arm.md (insn_enabled): Delete.
(enabled): Remove insn_enabled test.
(*arith_shiftsi): Delete. Replace with ...
(*<arith_shift_insn>_multsi): ... new pattern.
(*<arith_shift_insn>_shiftsi): ... new pattern.
* config/arm/arm.c (arm_print_operand): Handle operand format 'b'.
[-- Attachment #2: bootstrap.patch --]
[-- Type: text/plain, Size: 7130 bytes --]
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index ccad548..b514757 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -21271,7 +21271,15 @@ arm_print_condition (FILE *stream)
}
-/* If CODE is 'd', then the X is a condition operand and the instruction
+/* Globally reserved letters: acln
+ Puncutation letters currently used: @_|?().!#
+ Lower case letters currently used: bcdefhimpqtvwxyz
+ Upper case letters currently used: ABCDFGHJKLMNOPQRSTU
+ Letters previously used, but now deprecated/obsolete: sVWXYZ.
+
+ Note that the global reservation for 'c' is only for CONSTANT_ADDRESS_P.
+
+ If CODE is 'd', then the X is a condition operand and the instruction
should only be executed if the condition is true.
if CODE is 'D', then the X is a condition operand and the instruction
should only be executed if the condition is false: however, if the mode
@@ -21411,6 +21419,19 @@ arm_print_operand (FILE *stream, rtx x, int code)
}
return;
+ case 'b':
+ /* Print the log2 of a CONST_INT. */
+ {
+ HOST_WIDE_INT val;
+
+ if (!CONST_INT_P (x)
+ || (val = exact_log2 (INTVAL (x) & 0xffffffff)) < 0)
+ output_operand_lossage ("Unsupported operand for code '%c'", code);
+ else
+ fprintf (stream, "#" HOST_WIDE_INT_PRINT_DEC, val);
+ }
+ return;
+
case 'L':
/* The low 16 bits of an immediate constant. */
fprintf (stream, HOST_WIDE_INT_PRINT_DEC, INTVAL(x) & 0xffff);
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 348a89c..cd7495f 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -200,17 +200,9 @@
(const_string "yes")]
(const_string "no")))
-; Allows an insn to disable certain alternatives for reasons other than
-; arch support.
-(define_attr "insn_enabled" "no,yes"
- (const_string "yes"))
-
; Enable all alternatives that are both arch_enabled and insn_enabled.
(define_attr "enabled" "no,yes"
- (cond [(eq_attr "insn_enabled" "no")
- (const_string "no")
-
- (and (eq_attr "predicable_short_it" "no")
+ (cond [(and (eq_attr "predicable_short_it" "no")
(and (eq_attr "predicated" "yes")
(match_test "arm_restrict_it")))
(const_string "no")
@@ -9876,38 +9868,34 @@
\f
;; Patterns to allow combination of arithmetic, cond code and shifts
-(define_insn "*arith_shiftsi"
- [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
- (match_operator:SI 1 "shiftable_operator"
- [(match_operator:SI 3 "shift_operator"
- [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
- (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
- (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
+(define_insn "*<arith_shift_insn>_multsi"
+ [(set (match_operand:SI 0 "s_register_operand" "=r,r")
+ (shiftable_ops:SI
+ (mult:SI (match_operand:SI 2 "s_register_operand" "r,r")
+ (match_operand:SI 3 "power_of_two_operand" ""))
+ (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
"TARGET_32BIT"
- "%i1%?\\t%0, %2, %4%S3"
+ "<arith_shift_insn>%?\\t%0, %1, %2, lsl %b3"
+ [(set_attr "predicable" "yes")
+ (set_attr "predicable_short_it" "no")
+ (set_attr "shift" "4")
+ (set_attr "arch" "a,t2")
+ (set_attr "type" "alu_shift_imm")])
+
+(define_insn "*<arith_shift_insn>_shiftsi"
+ [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
+ (shiftable_ops:SI
+ (match_operator:SI 2 "shift_nomul_operator"
+ [(match_operand:SI 3 "s_register_operand" "r,r,r")
+ (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
+ (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
+ "TARGET_32BIT && GET_CODE (operands[3]) != MULT"
+ "<arith_shift_insn>%?\\t%0, %1, %3%S2"
[(set_attr "predicable" "yes")
(set_attr "predicable_short_it" "no")
(set_attr "shift" "4")
- (set_attr "arch" "a,t2,t2,a")
- ;; Thumb2 doesn't allow the stack pointer to be used for
- ;; operand1 for all operations other than add and sub. In this case
- ;; the minus operation is a candidate for an rsub and hence needs
- ;; to be disabled.
- ;; We have to make sure to disable the fourth alternative if
- ;; the shift_operator is MULT, since otherwise the insn will
- ;; also match a multiply_accumulate pattern and validate_change
- ;; will allow a replacement of the constant with a register
- ;; despite the checks done in shift_operator.
- (set_attr_alternative "insn_enabled"
- [(const_string "yes")
- (if_then_else
- (match_operand:SI 1 "add_operator" "")
- (const_string "yes") (const_string "no"))
- (const_string "yes")
- (if_then_else
- (match_operand:SI 3 "mult_operator" "")
- (const_string "no") (const_string "yes"))])
- (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_imm,alu_shift_reg")])
+ (set_attr "arch" "a,t2,a")
+ (set_attr "type" "alu_shift_imm,alu_shift_imm,alu_shift_reg")])
(define_split
[(set (match_operand:SI 0 "s_register_operand" "")
diff --git a/gcc/config/arm/iterators.md b/gcc/config/arm/iterators.md
index aebab93..1c41c6d 100644
--- a/gcc/config/arm/iterators.md
+++ b/gcc/config/arm/iterators.md
@@ -191,6 +191,20 @@
;; Right shifts
(define_code_iterator rshifts [ashiftrt lshiftrt])
+;; Binary operators whose second operand can be shifted.
+(define_code_iterator shiftable_ops [plus minus ior xor and])
+
+;; plus and minus are the only shiftable_ops for which Thumb2 allows
+;; a stack pointer opoerand. The minus operation is a candidate for an rsub
+;; and hence only plus is supported.
+(define_code_attr t2_binop0
+ [(plus "rk") (minus "r") (ior "r") (xor "r") (and "r")])
+
+;; The instruction to use when a shiftable_ops has a shift operation as
+;; its first operand.
+(define_code_attr arith_shift_insn
+ [(plus "add") (minus "rsb") (ior "orr") (xor "eor") (and "and")])
+
;;----------------------------------------------------------------------------
;; Int iterators
;;----------------------------------------------------------------------------
diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
index d74fcb3..032808c 100644
--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -291,6 +291,15 @@
|| ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32")))
(match_test "mode == GET_MODE (op)")))
+(define_special_predicate "shift_nomul_operator"
+ (and (ior (and (match_code "rotate")
+ (match_test "CONST_INT_P (XEXP (op, 1))
+ && ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32"))
+ (and (match_code "ashift,ashiftrt,lshiftrt,rotatert")
+ (match_test "!CONST_INT_P (XEXP (op, 1))
+ || ((unsigned HOST_WIDE_INT) INTVAL (XEXP (op, 1))) < 32")))
+ (match_test "mode == GET_MODE (op)")))
+
;; True for shift operators which can be used with saturation instructions.
(define_special_predicate "sat_shift_operator"
(and (ior (and (match_code "mult")
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: RFA: cache enabled attribute by insn code
2014-05-29 9:43 ` Richard Earnshaw
@ 2014-05-29 10:45 ` Richard Sandiford
0 siblings, 0 replies; 25+ messages in thread
From: Richard Sandiford @ 2014-05-29 10:45 UTC (permalink / raw)
To: Richard Earnshaw; +Cc: Christophe Lyon, Jeff Law, gcc-patches
Richard Earnshaw <rearnsha@arm.com> writes:
> On 27/05/14 16:07, Richard Sandiford wrote:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>> Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
>>>> Does the following patch help?
>>>
>>> Bah, it won't of course: %i1 needs to be the operator.
>>
>> Here's v2. I tested that it worked for simple tests like:
>>
>> int f1 (int x, int y) { return x + (y << 4); }
>> int f2 (int x, int y) { return x - (y << 4); }
>> int f3 (int x, int y) { return x & (y << 4); }
>> int f4 (int x, int y) { return x | (y << 4); }
>> int f5 (int x, int y) { return x ^ (y << 4); }
>> int f6 (int x, int y) { return (y << 4) - x; }
>> int g1 (int x, int y, int z) { return x + (y << z); }
>> int g2 (int x, int y, int z) { return x - (y << z); }
>> int g3 (int x, int y, int z) { return x & (y << z); }
>> int g4 (int x, int y, int z) { return x | (y << z); }
>> int g5 (int x, int y, int z) { return x ^ (y << z); }
>> int g6 (int x, int y, int z) { return (y << z) - x; }
>>
>> as well as the testcase.
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> * config/arm/iterators.md (shiftable_ops): New code iterator.
>> (t2_binop0, arith_shift_insn): New code attributes.
>> * config/arm/arm.md (insn_enabled): Delete.
>> (enabled): Remove insn_enabled test.
>> (*arith_shiftsi): Split out...
>> (*arith_multsi): ...this pattern and remove insn_enabled attribute.
>>
>
>
> Thanks, Richard.
> I've tweaked this as followed and committed it.
>
> I now consider "shift_operator" in the arm backend deprecated. We
> should be moving towards using shift_nomul_operator.
>
> There's one final wart still to be handled, though. 'rotate' can only
> take an immediate operand, not a register. We can currently deal with
> this, but it's not clean in terms of constraint handling. I'll see if I
> can fix this up sometime, but not today.
Thanks for picking it up. I realised later that I'd fluffed the
MULT check in:
> @@ -9876,38 +9868,34 @@
> \f
> ;; Patterns to allow combination of arithmetic, cond code and shifts
>
> -(define_insn "*arith_shiftsi"
> - [(set (match_operand:SI 0 "s_register_operand" "=r,r,r,r")
> - (match_operator:SI 1 "shiftable_operator"
> - [(match_operator:SI 3 "shift_operator"
> - [(match_operand:SI 4 "s_register_operand" "r,r,r,r")
> - (match_operand:SI 5 "shift_amount_operand" "M,M,M,r")])
> - (match_operand:SI 2 "s_register_operand" "rk,rk,r,rk")]))]
> +(define_insn "*<arith_shift_insn>_multsi"
> + [(set (match_operand:SI 0 "s_register_operand" "=r,r")
> + (shiftable_ops:SI
> + (mult:SI (match_operand:SI 2 "s_register_operand" "r,r")
> + (match_operand:SI 3 "power_of_two_operand" ""))
> + (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>")))]
> "TARGET_32BIT"
> - "%i1%?\\t%0, %2, %4%S3"
> + "<arith_shift_insn>%?\\t%0, %1, %2, lsl %b3"
> + [(set_attr "predicable" "yes")
> + (set_attr "predicable_short_it" "no")
> + (set_attr "shift" "4")
> + (set_attr "arch" "a,t2")
> + (set_attr "type" "alu_shift_imm")])
> +
> +(define_insn "*<arith_shift_insn>_shiftsi"
> + [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
> + (shiftable_ops:SI
> + (match_operator:SI 2 "shift_nomul_operator"
> + [(match_operand:SI 3 "s_register_operand" "r,r,r")
> + (match_operand:SI 4 "shift_amount_operand" "M,M,r")])
> + (match_operand:SI 1 "s_register_operand" "rk,<t2_binop0>,rk")))]
> + "TARGET_32BIT && GET_CODE (operands[3]) != MULT"
...this condition: operands[3] was the old numbering of the operator
rather than the new numbering. It looks like shift_nomul_operator
should make it redundant anyway.
Richard
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-05-29 10:45 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-20 8:16 RFA: cache enabled attribute by insn code Richard Sandiford
2014-05-20 8:17 ` Richard Sandiford
2014-05-20 14:42 ` Mike Stump
2014-05-20 17:50 ` Jeff Law
2014-05-20 21:37 ` Richard Sandiford
2014-05-23 18:19 ` Jeff Law
2014-05-27 13:12 ` Christophe Lyon
2014-05-27 13:37 ` Ramana Radhakrishnan
2014-05-27 14:59 ` Christophe Lyon
2014-05-27 14:08 ` Richard Sandiford
2014-05-27 14:18 ` Richard Sandiford
2014-05-27 15:07 ` Richard Sandiford
2014-05-27 16:36 ` Christophe Lyon
2014-05-28 14:01 ` Yufeng Zhang
2014-05-29 9:43 ` Richard Earnshaw
2014-05-29 10:45 ` Richard Sandiford
2014-05-27 15:15 ` Richard Earnshaw
2014-05-27 15:27 ` Jakub Jelinek
2014-05-27 15:40 ` Richard Earnshaw
2014-05-27 15:50 ` Jakub Jelinek
2014-05-27 16:01 ` Richard Earnshaw
2014-05-27 16:09 ` Richard Sandiford
2014-05-27 16:21 ` Richard Earnshaw
2014-05-27 16:31 ` Richard Sandiford
2014-05-28 15:11 ` Richard Earnshaw
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).