On Tue, Aug 23, 2016 at 12:10:29AM +0530, Prasad Ghangal wrote:
> On 22 August 2016 at 16:55, Trevor Saunders <tbsau...@tbsaunde.org> wrote:
> > On Sun, Aug 21, 2016 at 10:35:17PM +0530, Prasad Ghangal wrote:
> >> Hi all,
> >>
> >> As a part of my gsoc project. I have completed the following tasks:
> >>
> >> * Parsed gimple-expression
> >> * Parsed gimple-labels
> >> * Parsed local declaration
> >> * Parsed gimple-goto statement
> >> * Parsed gimple-if-else statement
> >> * Parsed gimple-switch statement
> >> * Parsed gimple-return statement
> >> * Parsed gimple-PHI function
> >> * Parsed gimple ssa-names along with default def
> >> * Parsed gimple-call
> >>
> >> * Hacked pass manager to add support for startwith (pass-name) to skip
> >> early opt passes
> >> * Modified gimple dump for making it parsable
> >>
> >> I am willing to continue work on the project, some TODOs for the projects
> >> are:
> >>
> >> * Error handling
> >> * Parse more gimple syntax
> >> * Add startwith support for IPA passes
> >>
> >> The complete code of gimple fe project can be found at
> >> https://github.com/PrasadG193/gcc_gimple_fe
> >>
> >>
> >> PFA patch for complete project (rebased for latest trunk revision).
> >> I have successfully bootstrapped and tested on x86_64-pc-linux-gnu.
> >> Some testcases failed due to modified gimple dump as expected.
> >>
> >>
> >> Thanks,
> >> Prasad
> >
> > only some rather minor comments
> >
> >
> > +++ b/gcc/c/c-parser.c
> > @@ -59,6 +59,18 @@ along with GCC; see the file COPYING3. If not see
> > #include "gimple-expr.h"
> > #include "context.h"
> > #include "gcc-rich-location.h"
> > +#include "tree-vrp.h"
> >
> > given that you need these headers it might be better to put most of the
> > gimple parsing in its own file so only what actually needs to know about
> > this part of the compiler does now about it.
> >
> > +void
> > +c_parser_parse_gimple_body (c_parser *parser)
> > +{
> > + bool return_p = false;
> > + gimple_seq seq;
> > + gimple_seq body;
> > + tree stmt = push_stmt_list ();
> >
> > it would be nice to move the declarations down to their first use.
> >
> > + gimple *ret;
> > + ret = gimple_build_return (NULL);
> >
> > there's no reason for a separate declaration and assignment ;)
> >
> > + tree block = NULL;
> > + block = pop_scope ();
> >
> > same here, and a number of other places.
> >
> > +c_parser_gimple_compound_statement (c_parser *parser, gimple_seq *seq)
> > +{
> > + bool return_p = false;
> > +
> > + if (!c_parser_require (parser, CPP_OPEN_BRACE, "expected %<{%>"))
> > + return return_p;
> >
> > return false would work fine.
> >
> > +
> > + if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE))
> > + {
> > + c_parser_consume_token (parser);
> > + goto out;
> >
> > I don't see the need for the gotos, there's no cleanup in this function.
> >
> > + /* gimple PHI expression. */
> > + if (c_parser_next_token_is_keyword (parser, RID_PHI))
> > + {
> > + c_parser_consume_token (parser);
> > +
> > + if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> > + {
> > + return;
> > + }
> > +
> > + gcall *call_stmt;
> > + tree arg = NULL_TREE;
> > + vec<tree> vargs = vNULL;
> >
> > I think you can use auto_vec here, as is I think this leaks the vectors
> > storage.
> >
> > +c_parser_gimple_binary_expression (c_parser *parser, enum tree_code
> > *subcode)
> >
> > you can skip the explicit 'enum' keyword.
> >
> > + struct {
> > + /* The expression at this stack level. */
> > + struct c_expr expr;
> >
> > similar with struct here.
> >
> > + /* The precedence of the operator on its left, PREC_NONE at the
> > + bottom of the stack. */
> > + enum c_parser_prec prec;
> > + /* The operation on its left. */
> > + enum tree_code op;
> > + /* The source location of this operation. */
> > + location_t loc;
> > + } stack[2];
> > + int sp;
> > + /* Location of the binary operator. */
> > + location_t binary_loc = UNKNOWN_LOCATION; /* Quiet warning. */
> > +#define POP
> > \
> >
> > it seems like it would be nicer to name the type, and then make this a
> > function.
> >
> > + RO_UNARY_STAR);
> > + ret.src_range.m_start = op_loc;
> > + ret.src_range.m_finish = finish;
> > + return ret;
> > + }
> > + case CPP_PLUS:
> > + if (!c_dialect_objc () && !in_system_header_at (input_location))
> > + warning_at (op_loc,
> > + OPT_Wtraditional,
> > + "traditional C rejects the unary plus operator");
> >
> > does it really make sense to warn about C issues when compiling gimple?
> >
> > +c_parser_parse_ssa_names (c_parser *parser)
> > +{
> > + tree id = NULL_TREE;
> > + c_expr ret;
> > + char *var_name, *var_version, *token;
> > + ret.original_code = ERROR_MARK;
> > + ret.original_type = NULL;
> > +
> > + /* ssa token string. */
> > + const char *ssa_token = NULL;
> > + ssa_token = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value);
> > + token = new char [strlen (ssa_token)];
> >
> > I'm not sure I see why you need this copy, and getting rid of it would
> > mean you don't need to free it.
> >
> > + strcpy (token, ssa_token);
> > +
> > + /* seperate var name and version. */
> > + var_version = strrchr (token, '_');
> > + if (var_version)
> > + {
> > + var_name = new char[var_version - token + 1];
> >
> > you should free this when done with it.
> >
> > +c_parser_gimple_postfix_expression (c_parser *parser)
> > +{
> > + struct c_expr expr;
> > + location_t loc = c_parser_peek_token (parser)->location;;
> >
> > extra ;
> >
> > + case CPP_OBJC_STRING:
> > + gcc_assert (c_dialect_objc ());
> > + expr.value
> > + = objc_build_string_object (c_parser_peek_token (parser)->value);
> > + set_c_expr_source_range (&expr, tok_range);
> > + c_parser_consume_token (parser);
> > + break;
> >
> > is there a reason to support objc stuff in gimple?
> >
> > +c_parser_gimple_expr_list (c_parser *parser, bool convert_p,
> > + vec<tree, va_gc> **p_orig_types,
> > + location_t *sizeof_arg_loc, tree *sizeof_arg,
> > + vec<location_t> *locations,
> > + unsigned int *literal_zero_mask)
> > +{
> > + vec<tree, va_gc> *ret;
> > + vec<tree, va_gc> *orig_types;
> > + struct c_expr expr;
> > + location_t loc = c_parser_peek_token (parser)->location;
> > + location_t cur_sizeof_arg_loc = UNKNOWN_LOCATION;
> > + unsigned int idx = 0;
> > +
> > + ret = make_tree_vector ();
> > + if (p_orig_types == NULL)
> > + orig_types = NULL;
> > + else
> > + orig_types = make_tree_vector ();
> > +
> > + if (sizeof_arg != NULL
> > + && c_parser_next_token_is_keyword (parser, RID_SIZEOF))
> > + cur_sizeof_arg_loc = c_parser_peek_2nd_token (parser)->location;
> > + if (literal_zero_mask)
> > + c_parser_check_literal_zero (parser, literal_zero_mask, 0);
> > + expr = c_parser_gimple_unary_expression (parser);
> > + if (convert_p)
> > + expr = convert_lvalue_to_rvalue (loc, expr, true, true);
> > + ret->quick_push (expr.value);
> >
> > That kind of relies on the details of make_tree_vector (), so it seems
> > somewhat safer to use vec_safe_push.
> >
> > + if (orig_types)
> > + orig_types->quick_push (expr.original_type);
> >
> > same
> >
> > +c_parser_gimple_declaration (c_parser *parser)
> > +{
> > + struct c_declspecs *specs;
> > + struct c_declarator *declarator;
> > + specs = build_null_declspecs ();
> > + c_parser_declspecs (parser, specs, true, true, true,
> > + true, true, cla_nonabstract_decl);
> > + finish_declspecs (specs);
> > + bool auto_type_p = specs->typespec_word == cts_auto_type;
> >
> > is it useful to support auto here in gimple?
> >
> > +c_parser_gimple_switch_stmt (c_parser *parser, gimple_seq *seq)
> > +{
> > + c_expr cond_expr;
> > + tree case_label, label;
> > + vec<tree> labels = vNULL;
> >
> > auto_vec?
> >
> > +static void
> > +c_finish_gimple_return (location_t loc, tree retval)
> > +{
> > + tree valtype = TREE_TYPE (TREE_TYPE (current_function_decl));
> > +
> > + /* Use the expansion point to handle cases such as returning NULL
> > + in a function returning void. */
> > + source_location xloc = expansion_point_location_if_in_system_header
> > (loc);
> > +
> > + if (TREE_THIS_VOLATILE (current_function_decl))
> > + warning_at (xloc, 0,
> > + "function declared %<noreturn%> has a %<return%>
> > statement");
> > +
> > + if (!retval)
> > + {
> > + current_function_returns_null = 1;
> > + if ((warn_return_type || flag_isoc99)
> >
> > I'm not sure what to do about warnings, but checking the language we are
> > compiling as seems kind of wrong when we're compiling gimple?
> >
> > @@ -228,6 +228,12 @@ struct GTY(()) function {
> > /* GIMPLE body for this function. */
> > gimple_seq gimple_body;
> >
> > + /* GIMPLEFE pass to start with */
> > + opt_pass *pass_startwith = NULL;
> >
> > I'm guessing you've only compiled in C++11 mode? because I'm pretty sure
> > you are using a C++11 feature here (the default member value you
> > assign).
> >
> > Thanks!
> >
> > Trev
> >
>
> Hi Trevor,
>
> Thanks for your feedback. I had missed removing some unwanted code
> while code cleanup. I have updated the patch.
> I am not sure if we should move all gimple parsing related functions
> to the new file (?)
I would think it makes sense to keep it all togehter, but probably not
terribly important where it goes.
> I am not getting what did you mean by C++11 mode (I am not explicitly
> giving any option while configure or make). I also have successfully
> bootstrapped and tested the project on another system. Is there any
> way to check that ?
consider this simplified example
struct foo
{
int *x = 0;
};
g++ -std=gnu++98 test.cc produces
/tmp/test.cc:3:11: warning: non-static data member initializers only available
with -std=c++11 or -std=gnu++11
int *x = 0;
^
gcc needs to compile as C++98 with compilers that don't understand this
construct, so you need to make sure the member is null when the struct
is created instead of putting = NULL next to the members declaration.
Thanks!
Trev
>
>
> Thanks,
> Prasad
