diff mbox

[PR68432,20/22] Record attributes for define_expand

Message ID 87mvu2fe9s.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Nov. 25, 2015, 12:35 p.m. UTC 
The define_subst support made it syntactically possible to add
attributes to a define_expand, but until now they had been ignored
by genattrtab.c.  This patch allows define_expands to have
"code,alternative" attributes but raises an error for general
"insn" attributes.   (Insn attributes would create a chicken-and-egg
problem because you'd need to call the expander to get the insn.)

The point is to allow define_expands to have preferred_for_size
and preferred_for_speed attributes.

Tested as described in the covering note.

gcc/
	* genattrtab.c (insn_def): Add max_type field.
	(check_defs): Check that the type of a define_expand attribute
	is acceptable.
	(gen_insn): Handle DEFINE_EXPAND.  Initialize the max_type field.
	(main): Handle DEFINE_EXPAND.

Comments

Bernd Schmidt Nov. 25, 2015, 3:55 p.m. UTC | #1 
On 11/25/2015 01:35 PM, Richard Sandiford wrote:
> The define_subst support made it syntactically possible to add
> attributes to a define_expand, but until now they had been ignored
> by genattrtab.c.  This patch allows define_expands to have
> "code,alternative" attributes but raises an error for general
> "insn" attributes.

This seems a little strange because define_expands don't really have 
alternatives. Also, using a string like that rather than some kind of 
identifier or a define_icode_attr maybe isn't the best approach?


Bernd
Richard Sandiford Nov. 25, 2015, 4:08 p.m. UTC | #2 
Bernd Schmidt <bschmidt@redhat.com> writes:
> On 11/25/2015 01:35 PM, Richard Sandiford wrote:
>> The define_subst support made it syntactically possible to add
>> attributes to a define_expand, but until now they had been ignored
>> by genattrtab.c.  This patch allows define_expands to have
>> "code,alternative" attributes but raises an error for general
>> "insn" attributes.
>
> This seems a little strange because define_expands don't really have 
> alternatives.

Yeah, but since an optab can be either a define_expand or a define_insn,
we have to support the case where there could more than one alternative.
For define_expands the alternative is always 0.

This is in some ways like an asm or define_insn with no constraints,
which also don't really have alternatives as such.  We just treat them
as having one alternative for consistency.  There's already an explicit
MAX in recog.c to force that.

> Also, using a string like that rather than some kind of 
> identifier or a define_icode_attr maybe isn't the best approach?

By "some kind of identifier" do you just mean replacing "code,alternative"
with a string that doesn't have a comma?  If so, I can do that, but what
should the string be?

The problem with define_icode_attr is that you get combinatorial
explosion with the type of the return value.  At the moment we just have
integers and enums (which can be defined by define_enum_attr as well as
define_attr), but who knows what we'll have in future? :-)

Also, I didn't want to call them "icode" attributes because in future
we might want attributes that really do depend only on the code, not on
the alternative.

Thanks,
Richard
Bernd Schmidt Nov. 26, 2015, 2:27 p.m. UTC | #3 
On 11/25/2015 05:08 PM, Richard Sandiford wrote:
>> Also, using a string like that rather than some kind of
>> identifier or a define_icode_attr maybe isn't the best approach?
>
> By "some kind of identifier" do you just mean replacing "code,alternative"
> with a string that doesn't have a comma?

Yeah. It really looks too much like the definition of attribute values IMO.

> The problem with define_icode_attr is that you get combinatorial
> explosion with the type of the return value.  At the moment we just have
> integers and enums (which can be defined by define_enum_attr as well as
> define_attr), but who knows what we'll have in future? :-)

Maybe

(define_typed_attr {enum,int} {insn,icode,icodealt}
  [all the usual stuff])


Bernd
diff mbox

Patch

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 6f797bf..42f6dc5 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -142,6 +142,7 @@  struct insn_def
   file_location loc;		/* Where in the .md files it occurs.  */
   int num_alternatives;		/* Number of alternatives.  */
   int vec_idx;			/* Index of attribute vector in `def'.  */
+  attr_type max_type;		/* Maximum allowable type.  */
 };
 
 /* Once everything has been read in, we store in each attribute value a list
@@ -1167,6 +1168,14 @@  check_defs (void)
 			XSTR (XEXP (value, 0), 0));
 	      continue;
 	    }
+	  if (id->max_type < attr->type)
+	    {
+	      error_at (id->loc, "`%s's cannot have %s attribute `%s'",
+			GET_RTX_NAME (GET_CODE (id->def)),
+			get_attr_type_name (attr->type),
+			attr->name);
+	      continue;
+	    }
 
 	  XVECEXP (id->def, id->vec_idx, i) = value;
 	  XEXP (value, 1) = check_attr_value (id->loc, XEXP (value, 1), attr);
@@ -3283,6 +3292,14 @@  gen_insn (md_rtx_info *info)
 
   switch (GET_CODE (def))
     {
+    case DEFINE_EXPAND:
+      id->insn_code = info->index;
+      id->insn_index = insn_index_number;
+      id->num_alternatives = 1;
+      id->vec_idx = 4;
+      id->max_type = AT_CODE_ALT;
+      break;
+
     case DEFINE_INSN:
       id->insn_code = info->index;
       id->insn_index = insn_index_number;
@@ -3290,6 +3307,7 @@  gen_insn (md_rtx_info *info)
       if (id->num_alternatives == 0)
 	id->num_alternatives = 1;
       id->vec_idx = 4;
+      id->max_type = AT_INSN;
       break;
 
     case DEFINE_PEEPHOLE:
@@ -3299,6 +3317,7 @@  gen_insn (md_rtx_info *info)
       if (id->num_alternatives == 0)
 	id->num_alternatives = 1;
       id->vec_idx = 3;
+      id->max_type = AT_INSN;
       break;
 
     case DEFINE_ASM_ATTRIBUTES:
@@ -3306,6 +3325,7 @@  gen_insn (md_rtx_info *info)
       id->insn_index = -1;
       id->num_alternatives = 1;
       id->vec_idx = 0;
+      id->max_type = AT_INSN;
       got_define_asm_attributes = 1;
       break;
 
@@ -5266,6 +5286,7 @@  main (int argc, char **argv)
     {
       switch (GET_CODE (info.def))
 	{
+	case DEFINE_EXPAND:
 	case DEFINE_INSN:
 	case DEFINE_PEEPHOLE:
 	case DEFINE_ASM_ATTRIBUTES: