Re: [PATCH] 15/19 modula2 front end: cc1gm2 additional non modula2 source files

[Gaius Mulley <gaiusmod2@gmail.com>]

Richard Biener <richard.guenther@gmail.com> writes:

>> +/* We don't use language_function.  */
>
> well ...

oops, yes - I'll remove the comment!

>> +struct GTY (()) language_function
>> +{
>> +
>> +  /* While we are parsing the function, this contains information about
>> +  the statement-tree that we are building.  */
>> +  /* struct stmt_tree_s stmt_tree;  */
>> +  tree stmt_tree;
>
> ... but this?
>
>> +};
>> +
>> +/* end of new stuff.  */
>> +
>> +/* Language hooks.  */
>> +
>> +bool
>> +gm2_langhook_init (void)
>> +{
>> +  build_common_tree_nodes (false);
>> +
>> +  /* I don't know why this has to be done explicitly.  */
>> +  void_list_node = build_tree_list (NULL_TREE, void_type_node);
>
> it's now done in build_common_tree_nodes

thanks

>> +  build_common_builtin_nodes ();
>> +
>> +  /* The default precision for floating point numbers.  This is used
>> +     for floating point constants with abstract type.  This may eventually
>> +     be controllable by a command line option.  */
>> +  mpfr_set_default_prec (256);
>> +
>> +  /* GNU Modula-2 uses exceptions.  */
>> +  using_eh_for_cleanups ();
>> +
>> +  return true;
>> +}
>> +
>> +/* The option mask.  */
>> +
>> +static unsigned int
>> +gm2_langhook_option_lang_mask (void)
>> +{
>> +  return CL_ModulaX2;
>> +}
>> +
>> +/* Initialize the options structure.  */
>> +
>> +static void
>> +gm2_langhook_init_options_struct (struct gcc_options *opts)
>> +{
>> +  /* Default to avoiding range issues for complex multiply and divide.  */
>> +  opts->x_flag_complex_method = 2;
>> +
>> +  /* The builtin math functions should not set errno.  */
>> +  opts->x_flag_errno_math = 0;
>> +  opts->frontend_set_flag_errno_math = true;
>> +
>> +  /* Exceptions are used to handle recovering from panics.  */
>> +  opts->x_flag_exceptions = 1;
>> +  opts->x_flag_non_call_exceptions = 1;
>
> whohoo - really non-call-exceptions?

ah thankyou.  Now removed, bootstrapped and regression tests pass.

>> +  init_FrontEndInit ();
>> +}
>> +

[snip]

>> +static tree
>> +gm2_langhook_type_for_mode (machine_mode mode, int unsignedp)
>> +{
>> +  tree type;
>> +
>> +  if (VECTOR_MODE_P (mode))
>> +    {
>> +      tree inner;
>> +
>> +      inner = gm2_langhook_type_for_mode (GET_MODE_INNER (mode), unsignedp);
>> +      if (inner != NULL_TREE)
>> +        return build_vector_type_for_mode (inner, mode);
>> +      return NULL_TREE;
>> +    }
>> +
>> +  scalar_int_mode imode;
>> +  scalar_float_mode fmode;
>> +  complex_mode cmode;
>> +  if (is_int_mode (mode, &imode))
>> +    return gm2_langhook_type_for_size (GET_MODE_BITSIZE (imode), unsignedp);
>> +  else if (is_float_mode (mode, &fmode))
>> +    {
>> +      switch (GET_MODE_BITSIZE (fmode))
>> +        {
>> +        case 32:
>> +          return float_type_node;
>> +        case 64:
>> +          return double_type_node;
>
> Have a look at lto/lto-lang.cc where we match the global types with

thanks will do!

>   if (mode == TYPE_MODE (float_type_node))
>     return float_type_node;
>
> I think that's better than relying on the size statically as you do
> above.

yes indeed

>> +        default:
>> +          // We have to check for long double in order to support
>> +          // i386 excess precision.
>> +          if (fmode == TYPE_MODE (long_double_type_node))
>> +            return long_double_type_node;
>> +        }
>> +    }
>> +  else if (is_complex_float_mode (mode, &cmode))
>> +    {
>> +      switch (GET_MODE_BITSIZE (cmode))
>> +        {
>> +        case 64:
>> +          return complex_float_type_node;
>> +        case 128:
>> +          return complex_double_type_node;
>> +        default:
>> +          // We have to check for long double in order to support
>> +          // i386 excess precision.
>> +          if (cmode == TYPE_MODE (complex_long_double_type_node))
>> +            return complex_long_double_type_node;
>> +        }
>> +    }
>> +
>> +#if HOST_BITS_PER_WIDE_INT >= 64
>> +
>> +  /* The middle-end and some backends rely on TImode being supported
>> +  for 64-bit HWI.  */
>> +  if (mode == TImode)
>> +    {
>> +      type = build_nonstandard_integer_type (GET_MODE_BITSIZE (TImode),
>> +                                             unsignedp);
>> +      if (type && TYPE_MODE (type) == TImode)
>> +        return type;
>> +    }
>> +#endif
>
> Instead of this block look at c-family/c-common.cc which does
>
>   for (i = 0; i < NUM_INT_N_ENTS; i ++)
>     if (int_n_enabled_p[i]
>         && mode == int_n_data[i].m)
>       return (unsignedp ? int_n_trees[i].unsigned_type
>               : int_n_trees[i].signed_type);

ok will do - thanks for the direction.

> it might be practical to factor out handling of the global tree nodes into
> a function in the middle-end that frontends can call after processing modes
> it has special types for.

yes it sounds like a common front end use.

>> +  return NULL_TREE;
>> +}
>> +

[snip]

>> +/* m2_write_global_declarations writes out globals by coping into a vec
>> +   and calling wrapup_global_declarations.  */
>> +
>> +static void
>> +m2_write_global_declarations (tree globals)
>> +{
>> +  tree decl = globals;
>> +  int n = 0;
>> +
>> +  while (decl != NULL)
>> +    {
>> +      n++;
>> +      decl = TREE_CHAIN (decl);
>> +    }
>> +
>> +  if (n > 0)
>> +    {
>> +      int i = 0;
>> +      tree vec[n];
>
> to simplify this and to avoid huge stack usage it might
> be easier to use an auto_vec<tree> here and simply
> pushing the chain onto that, passing .address () to
> wrapup_global_declarations.

will do.

>> +      decl = globals;
>> +      while (decl != NULL)
>> +        {
>> +          vec[i] = decl;
>> +          decl = TREE_CHAIN (decl);
>> +          i++;
>> +        }
>> +      wrapup_global_declarations (vec, n);
>> +    }
>> +}

[snip]

>> +/* FIXME: This is a hack to preserve trees that we create from the
>> +   garbage collector.  */
>> +
>> +static GTY (()) tree gm2_gc_root;
>> +static tree personality_decl = NULL_TREE;
>> +
>> +static void
>> +gm2_preserve_from_gc (tree t)
>> +{
>> +  gm2_gc_root = tree_cons (NULL_TREE, t, gm2_gc_root);
>
> I suppose it's difficult to properly mark roots in the m2 sources?

yes - especially if they are translated into C++ (not impossible - but
easier do it in a C++ module and then make m2 call the C++ function).

> I'll note that using a tree_list is prone to deep GC mark stacks,
> eventually a vec<tree, va_gc> might be more efficient here.

thanks I'll change this and avoid tree_list in favour of vec in the future.

> Since more frontends seem to use hacks like this "support"
> for this from the GC itself might be nice (even if just having
> this "hackish" GC root globally).  Just an idea for the future.

yes this sounds cleaner.

>> +}
>> +
>> +/* Return a decl for the exception personality function.  */
>> +
>> +static tree
>> +gm2_langhook_eh_personality (void)
>> +{
>> +  if (personality_decl == NULL_TREE)
>> +    {
>> +      personality_decl = build_personality_function ("gxx");
>> +      gm2_preserve_from_gc (personality_decl);
>
> For example the C++ frontend just has a global
>
> static GTY(()) tree cp_eh_personality_decl;

thanks - I'll re-examine/borrow the C++ code.

>> +    }
>> +  return personality_decl;
>> +}
>> +
>> +/* Functions called directly by the generic backend.  */
>
>> +tree
>> +convert_loc (location_t location, tree type, tree expr)
>> +{
>> +  if (type == error_mark_node || expr == error_mark_node
>> +      || TREE_TYPE (expr) == error_mark_node)
>> +    return error_mark_node;
>> +
>> +  if (type == TREE_TYPE (expr))
>> +    return expr;
>> +
>> +  gcc_assert (TYPE_MAIN_VARIANT (type) != NULL);
>> +  if (TYPE_MAIN_VARIANT (type) == TYPE_MAIN_VARIANT (TREE_TYPE (expr)))
>> +    return fold_convert (type, expr);
>> +
>> +  expr = m2convert_GenericToType (location, type, expr);
>> +  switch (TREE_CODE (type))
>> +    {
>> +    case VOID_TYPE:
>> +    case BOOLEAN_TYPE:
>> +      return fold_convert (type, expr);
>> +    case INTEGER_TYPE:
>> +      return fold (convert_to_integer (type, expr));
>> +    case POINTER_TYPE:
>> +      return fold (convert_to_pointer (type, expr));
>> +    case REAL_TYPE:
>> +      return fold (convert_to_real (type, expr));
>> +    case COMPLEX_TYPE:
>> +      return fold (convert_to_complex (type, expr));
>> +    case ENUMERAL_TYPE:
>> +      return fold (convert_to_integer (type, expr));
>> +    default:
>> +      error_at (location, "cannot convert expression, only base types can be converted");
>> +      break;
>> +    }
>> +  return error_mark_node;
>> +}
>> +
>> +/* Functions called directly by the generic backend.  */
>
> In theory, if the frontend doesn't call convert_* from gcc/convert.{h,cc} then
> these shouldn't be necessary and are never called (but we build and link
> convert.cc so they have to be present) - does the m2 frontend use
> those?

yes.  It might be possible to reduce the uses of convert_* in the front
end though.

> If not it _shoult_ work to simply have a gcc_unreachable () in them ..
> (and we should have a LANG_USES_CONVERT language macro to gate
> compiling of convert.cc or simply adjust all the lang/Make-lang.in to include
> it when necessary, moving convert.{cc,h} to a new gcc/lang-common/
> directory).

ok.

>> +tree
>> +convert (tree type, tree expr)
>> +{
>> +  return convert_loc (m2linemap_UnknownLocation (), type, expr);
>> +}
>> +

[snip]

>> +tree
>> +gm2_type_for_size (unsigned int bits, int unsignedp)
>> +{
>> +  tree type;
>> +
>> +  if (unsignedp)
>> +    {
>> +      if (bits == INT_TYPE_SIZE)
>> +        type = unsigned_type_node;
>> +      else if (bits == CHAR_TYPE_SIZE)
>> +        type = unsigned_char_type_node;
>> +      else if (bits == SHORT_TYPE_SIZE)
>> +        type = short_unsigned_type_node;
>> +      else if (bits == LONG_TYPE_SIZE)
>> +        type = long_unsigned_type_node;
>> +      else if (bits == LONG_LONG_TYPE_SIZE)
>> +        type = long_long_unsigned_type_node;
>> +      else
>> +        type = make_unsigned_type (bits);
>
> You want to use build_nonstandard_integer_type instead
> here, otherwise each call gets a distinct type which is probably
> not intended.  Frontends are fine to return NULL_TREE for
> unhandled cases though.

ah thanks - will change this.

> Again the c-family/c-common.cc or the lto-lang.cc implementations
> might be worth looking at.
>
>> +    }
>> +  else
>> +    {
>> +      if (bits == INT_TYPE_SIZE)
>> +        type = integer_type_node;
>> +      else if (bits == CHAR_TYPE_SIZE)
>> +        type = signed_char_type_node;
>> +      else if (bits == SHORT_TYPE_SIZE)
>> +        type = short_integer_type_node;
>> +      else if (bits == LONG_TYPE_SIZE)
>> +        type = long_integer_type_node;
>> +      else if (bits == LONG_LONG_TYPE_SIZE)
>> +        type = long_long_integer_type_node;
>> +      else
>> +        type = make_signed_type (bits);
>> +    }
>> +  return type;
>> +}
>> +

[snip]

gcc/m2/m2pp.cc

>> +static void m2pp_decl_list (pretty *s, tree t);
>> +static void m2pp_loc (pretty *s, tree t);
>> +
>> +void pet (tree t);
>> +void m2pp_integer (pretty *s, tree t);
>> +
>> +extern void stop (void);
>> +
>> +static stack *stackPtr = NULL;
>> +
>> +/* do_pf helper function for pf.  */
>> +
>> +void
>> +do_pf (tree t, int bits)
>
> Can you put these inside a namespace?  Short identifiers tend to

sure yes will do

> cause "issues", esp. the two letter ones below.  If they are merely
> for convenience in gdb sessions amending the gdbinit.in or
> gdbhooks.py might be more appropriate (not sure if we can have
> language specific snippets in there)

yes they were for gdb convenience - I'll look into gdbhooks.py.

[snip]

>> diff -ruw /dev/null gcc-git-devel-modula2/gcc/m2/version.c
>> --- /dev/null   2022-08-24 16:22:16.888000070 +0100
>> +++ gcc-git-devel-modula2/gcc/m2/version.c      2022-10-07 20:21:18.682097332 +0100
>> @@ -0,0 +1 @@
>> +#define version_string  "1.9.5"
>
> The rest looks OK.  I wonder if caret diagnostics work for modula2
> (for middle-end emitted
> diagnostics, not sure if the frontend uses GCCs diagnostic machinery)

yes, the front end (after lexical analysis) uses GCCs location_t and
emits: notes, warnings, errors and generates virtual locations for
operator sub expressions etc (using GCCs diagnostic machinery).  I hope
to submit an analyzer patch for m2 during the next stage1.

The front end attempts to pass well formed non error trees down to
gimple (effort has been taken to cast and convert trees to avoid
middle/back end warnings).  But it might be cleaner to have a
middle/back end call back so that front end knowledge can be relayed in
error messages.

Thank you for reviewing the patch!

regards,
Gaius