On Mon, Aug 22, 2016 at 8:40 PM, Prasad Ghangal <prasad.ghan...@gmail.com> 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 think it might be good to make the parts of the C parser you use more obvious (you'd need to export functions like c_parser_next_token_is). The easiest way to "force" that is to put all of the gimple parsing into a separate file. Note I am not so much concerned about this at the moment, the parts to improve would be avoiding more of the C-isms like convert_lvalue_to_rvalue, handling of SIZEOF_EXPR and other stuff that looks redundant (you've probably copied this from the C parsing routines and refactored it). Also the GIMPLE parser shouldn't do any warnings (just spotted a call to warn_for_memset). Thanks, Richard. > 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 ? > > > Thanks, > Prasad
