Re: [gimplefe] patch

[Diego Novillo <dnovillo at google dot com>]

Ketaki,

Some comments on your patch.  Apologies for the delay.  It's going in
the right direction, though it needs several adjustments.

Could you please add the document you sent me for the grammar to the
wiki page?  It's better if we edit the grammar there instead of having
a separate document.


> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 175674)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,10 @@
> +2011-07-14 Ketaki Tiwatne <ketaki.08@gmail.com>

Two spaces between date and name.

> +     *parser.c
> +     (gp_parse_var_decl): Addition of attributes to variable declaration.
> +     (gp_parse_function_decl): New.
> +     (gp_parse_decl): Addition of function declaration case.
> +     Shifted few lines of code from gp_parse_var_decl.
> +
>  2011-05-13  Sandeep Soni  <soni.sandeepb@gmail.com>
>
>       * parser.c (gl_gimple_code_for_token): New.
> Index: parser.c
> ===================================================================
> --- parser.c  (revision 175674)
> +++ parser.c  (working copy)
> @@ -130,7 +130,7 @@ static bool
>  gl_token_starts_decl (gimple_token *token)
>  {
>    enum tree_code code = gl_tree_code_for_token (token);
> -  return code == VAR_DECL;
> +  return code == VAR_DECL || FUNCTION_DECL;

Let's prepare this for all kinds of decls, change to

        return TREE_CODE_CLASS (code) == tcc_declaration;

>  }
>
>
> @@ -813,7 +813,7 @@ gp_parse_type (gimple_parser *parser, const gimple
>
>     The syntax of a variable declaration is as follows:
>
> -   <VAR_DECL<Name, Type>>
> +   VAR_DECL<Name <Type <Attributes>>>

Why the change in the tuple?  The VAR_DECL tuple has two
elements: name and type.  I would document it as:

VAR_DECL <name, type>

TYPE is a tuple with 1 required argument and 1 or more
optional attributes:

type <size, [type_attributes]>

TYPE_ATTRIBUTES is a comma-separated list of optional modifiers
to the type:

        - sign=[signed|unsigned] (default: signed)
        - readonly=[true|false] (default: false)
        [ ... others to come ... ]

>     Following are the broad cases for which the syntax of a variable
>     declaration is described:
> @@ -821,31 +821,46 @@ gp_parse_type (gimple_parser *parser, const gimple
>     Example:
>
>     1. C-like declaration as,
> -     int var;
> +      a. int var;
>
> -   The Corresponding gimple syntax,
> -     VAR_DECL <var, INTEGER_TYPE <4>>
> +      The Corresponding gimple syntax,

s/Corresponding/corresponding/

> +        VAR_DECL < var < INTEGER_TYPE < 4, sign=signed, readonly=false > > >

VAR_DECL <var, INTEGER_TYPE <4, sign=signed, readonly=false>>>

(Similar fixes in the other examples).

>
> -   In General,any variable of an atomic data type,
> -     VAR_DECL <var_name, TYPE <size>>
> +      b. unsigned long int var;
>
> +      The Corresponding gimple syntax,
> +        VAR_DECL < var < INTEGER_TYPE < 8, sign=unsigned, readonly=false > > >
> +
> +      c. const char var;
> +
> +      The Corresponding gimple syntax,
> +        VAR_DECL < var < INTEGER_TYPE < 1, sign=signed, readonly=true > > >
> +
> +      d. float var;
> +
> +      The Corresponding gimple syntax,
> +        VAR_DECL < var < REAL_TYPE < 4, sign=signed, readonly=false > > >
> +
> +      In General,any variable of an atomic data type,
> +        VAR_DECL <var_name <TYPE <size sign=<signed/unsigned> readonly=<false/true>>>>

Watch out for line wrapping.  All lines should fit in 80 columns.
This happens in various places.  I've only marked this one.

> +   Note: The space between the two consecutive '>' is needed in this
> +      implementation since the token is consumed is CPP_GREATER.
> +         TODO: This can be improved if needed.

Hm, I'm wondering whether we should use a different delimiter.
How about we switch to '(' and ')'?

>
> +   Recognizer function for variable declarations. The declaration tuple is read
> +   from gimple_parser PARSER.
> +*/
> +
>  static void
>  gp_parse_var_decl (gimple_parser *parser)
>  {
>    const gimple_token *next_token;
>    enum tree_code code ;
>
> -  gl_consume_expected_token (parser->lexer, CPP_LESS);
> -  gl_consume_expected_token (parser->lexer, CPP_NAME);
> -  gl_consume_expected_token (parser->lexer, CPP_COMMA);
> -
>    next_token = gl_consume_token (parser->lexer);
>    code = gl_tree_code_for_token (next_token);
> +
>    switch (code)
>      {
> +/* Case for Integer and Characters */
>      case INTEGER_TYPE:
> +/* Case for float and double */

Unnecessary comment.  Also comments go just before the code, not
between 'case' labels.

> +    case REAL_TYPE:
> +      gl_consume_expected_token (parser->lexer, CPP_LESS);
> +/* size */

Align comment with the code.  Also, factor this whole switch
statement into a separate function gp_parse_type.

>        gl_consume_expected_token (parser->lexer, CPP_NUMBER);
> -      gl_consume_expected_token (parser->lexer, CPP_RSHIFT);
> +      gl_consume_expected_token (parser->lexer, CPP_COMMA);
> +/* sign = signed/unsigned */
> +      gl_consume_expected_token (parser->lexer, CPP_NAME);
> +      gl_consume_expected_token (parser->lexer, CPP_EQ);
> +      gl_consume_expected_token (parser->lexer, CPP_NAME);
> +      gl_consume_expected_token (parser->lexer, CPP_COMMA);

No.  This makes signed a required attribute.  All the attributes
should be optional.

You will need to implement a data structure to keep track of the
type and attributes that you are parsing.  Once you finish the
parse, you will then use this collected data to instantiate types
with make_*_type (see, make_signed_type for instance).

Note, however, that we will need to recognize whether we are
requested to instantiate a type that already exists.  So, you
will need to keep a cache of instantiated types and pre-fill that
cache with the primitive types: integer_type_node,
unsigned_type_node, etc.

To lookup types in this cache, you will likely want a hash table
that takes attributes like size and sign as input and maps them
to a type node.


> +static void
> +gp_parse_function_decl (gimple_parser *parser)
> +{
> +  gl_consume_expected_token (parser->lexer, CPP_LESS);
> +  gl_consume_expected_token (parser->lexer, CPP_NAME);
> +  gl_consume_expected_token (parser->lexer, CPP_LESS);
> +/* consume FUNCTION_TYPE */
> +  gl_consume_token(parser->lexer);
> +/* return type */

I expect you would be calling gp_parse_type() from here.

> +  gl_consume_expected_token (parser->lexer, CPP_LESS);
> +  gp_parse_var_decl(parser);
> +  gl_consume_expected_token (parser->lexer, CPP_GREATER);
> +/* argument count */
> +  gl_consume_expected_token (parser->lexer, CPP_NUMBER);
> +  gl_consume_expected_token (parser->lexer, CPP_GREATER);
> +  gl_consume_expected_token (parser->lexer, CPP_GREATER);
> +}
> +
>  /* The token TOKEN read from the reader PARSER is looked up for a match.
>     Calls the corresponding function to do the parsing for the match.
>     Gets called for recognizing variable and function declarations. */
> @@ -941,9 +1006,17 @@ gp_parse_decl (gimple_parser *parser, const gimple
>    switch (code)
>      {
>      case VAR_DECL:
> +      gl_consume_expected_token (parser->lexer, CPP_LESS);
> +/* Variable name */
> +      gl_consume_expected_token (parser->lexer, CPP_NAME);
> +      gl_consume_expected_token (parser->lexer, CPP_LESS);

No. Leave this in gp_parse_var_decl.

>        gp_parse_var_decl (parser);

> +      gl_consume_expected_token (parser->lexer, CPP_GREATER);
> +      gl_consume_expected_token (parser->lexer, CPP_GREATER);

Likewise.


Diego.