Re: define_constraints patch, revised

[Zack Weinberg <zackw at panix dot com>]

On Sun, Feb 19, 2006 at 04:25:56AM +0100, Bernd Schmidt wrote:
> Zack Weinberg wrote:
> >Over the weekend I finally had time to revise my patch to enable defining
> >constraints in the machine description.
> 
> On the whole I really like this.

Yay!

> > * Some bugs are fixed.  (And I now know *why* Jan changed the SSE constant
> >   constraint from 'H' to 'C'. :-)
> 
> Do tell (oh, it's a CONST_DOUBLE thingy?).

Yes.  'H' can only match CONST_DOUBLEs, but some SSE constants are
CONST_VECTORs.  By me this is a compelling argument for relaxing the
rules on GHIJKLMNOP...

> > 	(Machine Constraints): Use pathnames relative to gcc directory,
> 
> This doesn't seem related to the rest of the patch?  Ok separately; the 
> manual doesn't seem consistent but this seems like the sensible 
> direction to change it.

It's necessary for the patch, because converted ports change from
referencing ARCH.h to referencing config/ARCH/predicates.md; you're
right that the manual is not consistent overall, but it was consistent
within this one section, so I thought it best to change it all at once.
I can certainly split this out and check it in by itself.

> >+If an operand constraint string could be broken into individual
> >+constraints more than one way, the longest match wins: for example, if
> >+@code{x}, @code{y}, and @code{xy} are all defined as constraint names,
> >+the text @samp{xy} in an operand constraint will be read as the single
> >+constraint @code{xy}, not the two constraints @code{x} and @code{y}.
> 
> I'd prefer this situation to be an error at genpreds runtime.

Hmm, do you mean the situation where one constraint name is a proper
prefix of another, or where there's an actual ambiguous operand constraint?
The latter would be rather painful to implement.  I was worried that the
former would break some existing port, but having looked through them all
it doesn't appear that any of them currently define constraints that collide
in this fashion (I'm not 100% sure about s390...) so I guess that would
be fine.

Incidentally, while looking through all the ports I noticed that bfin
has a bug in its REG_CLASS_FROM_LETTER: 'B' is being mapped to two
different regclasses, ODD_AREGS and BREGS.  (It's a ?: chain, so it
will always resolve to ODD_AREGS...)

> >+@deffn {MD Expression} define_register_constraint name regclass docstring
> >+All three arguments are string constants.
> >+@var{name} is the name of the predicate, as it will appear in
>                                  ^ constraint
> 
> That had me confused for a while.

Oops!  Thanks, will correct.

> >+If you put the pseudo-Texinfo directive @samp{@@internal} at the
> >+beginning of a docstring, then (in the future) it will appear only in
> >+the internals manual's version of the machine-specific constraint
> >+tables.  Use this for constraints that should not appear in @code{asm}
> >+statements.  (The compiler itself is unaware of the distinction; this
> >+is not a way to give users a helpful diagnostic instead of a
> >+@code{reload} failure.)
> 
> Can't we extend the syntax for define_*_constraint to add an "internal" 
> flag somehow?  Sounds like there's a useful feature waiting to be 
> discovered here.

Well, I'm not sure what other syntax is feasible (suggestions
welcome), and I don't see why whatever non-manual use we found
couldn't just look at the docstring.

> >If the name does
> >+contain angle brackets, each @samp{<} is replaced with @samp{lt}, each
> >+@samp{>} with @samp{gt}, and a leading underscore is added:
> >+@code{CONSTRAINT__P4gtX} for @code{P4>X}, for instance.
> 
> Doesn't this introduce ambiguities, if you have a constraint string such 
> as "P4gt>"?  That may be an unlikely thing to worry about, but still I'd 
> prefer a mangling that replaced "_" by "__", "<" by "_lt" and ">" by "_gt".

I'm not sure I understand the question.  Mangling only applies to
the enumeration constants for *individual* constraint names, not to
operand constraint strings.  If you mean "P4gt>" to be an individual name,
well, it would be mangled to CONSTRAINT__P4gtgt. "P4gtgt" would be
CONSTRAINT_P4gtgt, with one fewer underscore.  This is why one is not
allowed to begin constraint names with an underscore.  I *think* this is
bulletproof.  However, your suggested mangling might be easier for humans
going between constraint names and enumerator names, so I don't mind
changing it, but could you please clarify which situation you are worried
about?

> >+   2: A docstring for this constraint, in Texinfo syntax; will be
> >+      incorporated into the manual's list of machine-specific operand
> >+      constraints.  */
> 
> "... in future versions"?

Ya, good catch.  I carefully made this clear in the manual but forgot
about the comments here.

> >+   used by generic constraints.  The letters not in the list are:
> >+   EFVXgimnops.  */
> 
> Also 'r'.  The array definition looks fine.

Will change.

> >+  c = obstack_alloc (rtl_obstack, sizeof (struct constraint_data));
> >+  c->name         = name;
> >+  c->c_name	  = need_mangled_name ? mangle (name) : name;
> >+  c->namelen      = strlen (name);
> >+  c->regclass     = regclass;
> >+  c->exp          = exp;
> >+  c->is_register  = c->regclass != 0;
> >+  c->is_const_int = strchr (const_int_constraints, name[0]) != 0;
> >+  c->is_const_dbl = strchr (const_dbl_constraints, name[0]) != 0;
> >+  c->is_extra     = !(c->is_register || c->is_const_int || 
> >c->is_const_dbl);
> >+  c->is_memory    = is_memory;
> >+  c->is_address   = is_address;
> 
> Nonstandard formatting.  Elsewhere too.

With long lists of assignments, I like to line up the equals signs.  I
can change this if you really want.

> >+      if (c->is_memory)
> >+	message_with_line (lineno, "warning: '%s' will not be treated as a "
> >+			   "memory constraint, due to its name", name);
> >+      else if (c->is_address)
> >+	message_with_line (lineno, "warning: '%s' will not be treated as an "
> >+			   "address constraint, due to its name", name);
> 
> Better error out, I think.

Okay.

> >+  puts ("bool\n"
> >+	"constraint_satisfied_p (rtx op, enum constraint_num c)\n"
> >+	"{\n"
> >+	"  enum machine_mode mode = GET_MODE (op);\n"
> >+	"  HOST_WIDE_INT ival = 0;\n"
> >+	"  HOST_WIDE_INT hval = 0;\n"
> >+	"  unsigned HOST_WIDE_INT lval = 0;\n"
> >+	"  const REAL_VALUE_TYPE *rval = 0;\n"
> >+	"\n"
> >+	"  if (GET_CODE (op) == CONST_INT)\n"
> >+	"    ival = INTVAL (op);\n"
> >+	"  else if (GET_CODE (op) == CONST_DOUBLE)\n"
> >+	"    {\n"
> >+	"      if (mode == VOIDmode)\n"
> >+	"        hval = CONST_DOUBLE_HIGH (op),\n"
> >+	"        lval = CONST_DOUBLE_LOW  (op);\n"
> >+	"      else\n"
> >+	"        rval = CONST_DOUBLE_REAL_VALUE (op);\n"
> >+	"    }\n"
> 
> Ewww.
> 
> If you're going to have a switch and a function call anyway, my 
> preferred technique for this kind of thing is
> 
> bool noinline constraint_satisfied_I (...) { HWI ival = ...; return 
> expr_for_I; }
> bool noinline constraint_satisfied_c (...) { return expr_for_c; }
> 
> inline constraint_satisfied_p (...)
> {
> switch (GET_CODE (op))
>  { case I: return constraint_satisfied_I (...);
>    case c: return constraint_satisfied_c (...);
>  }
> }
> 
> That would also allow machine descriptions to directly call the separate 
> constraint_satisfied_foo functions (sort of like gen_rtx_FOO vs gen_rtx).

... switch on GET_CODE (op)?  I don't think that's what you meant.  I see
the appeal of one function per constraint, but I'm not sure I can
clean up the [ihlr]val assignments with that; the code that writes
out test expressions is not set up to put assignments inside
subexpressions.  And it may not be evident from genpreds' point of
view whether a given expression can make safe use of one of these;
consider something like 

(define_constraint "T" "..."
  (and (match_test "some_function_in_arch_c (op)")
       (match_test "ival <= 128")))

where some_function_in_arch_c only ever returns true for CONST_INTs.

Note that even if I did break up constraint_satisfied_p into
a set of satisfies_constraint_XXX functions, we would need to keep
constraint_satisfied_p and all the other functions that take an
enum constraint_num, because they're necessary to implement the old
interface that MI code still uses.  I'm up for cleaning up that
interface, but only after all ports use the new framework.

> >+/* This is a complete list (unlike the one in genpreds.c) of constraint
> >+   letters and modifiers with machine-independent meaning.  The only
> >+   omission is digits, as these are handled specially.  */
> >+static const char indep_constraints[] = ",=+%*?!#&<>pmVoEFsinXgr";
> 
> If it was alphabetical it would be easier to compare to the one in 
> genpreds.c.  It does seem to match.

Will change.  I believe this is the order of case entries in reload.c.

> >+/* This is a list of all the letters which may legitimately begin
> >+   the names of constraints defined by the machine description.  */
> >+static const char mdep_constraint_letters[] =
> >+"ABCDGHIJKLMNOPQRSTUWYZabcdefhjklqtuvwxyz";
> 
> Identical string constant used in two places - move to header file?

I guess I could throw it in gensupport.h.

> >+  for (p = &constraints_by_letter_table[i]; *p; p = 
> >&(*p)->next_this_letter)
> >+    {
> >+      if ((*p)->namelen < namelen)
> >+	break;
> >+      if ((*p)->namelen == namelen
> >+	  && !strcmp (name, (*p)->name))
> >+	{
> >+	  error ("constraint '%s' defined more than once", name);
> >+	  return;
> >+	}
> >+    }
> 
> Move all error checking to add_constraint in genpreds.c?

I don't have the code in front of me right now, but I believe this check
is also in genpreds.c; genoutput.c has to have enough error checking to
be robust in the face of malformed input, and there really is nothing
sensible to do here besides error out.

zw