On Fri, Aug 26, 2016 at 5:08 AM, Prasad Ghangal <prasad.ghan...@gmail.com> wrote: > On 24 August 2016 at 15:32, Richard Biener <richard.guent...@gmail.com> wrote: >> 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). >> > PFA updated patch (successfully bootstrapped and tested on > x86_64-pc-linux-gnu). I have removed unnecessary code. On side I am > also trying to move gimple parser related functions to new file. But > for it we also have to move structs like c_token, c_parser. Won't it > disturb the c-parser code structure ?
Yeah, as I said it would be nice but it might be quite some work. I'd move such stuff into a new c-parser.h header that can be included by the gimple parser file. Note existing exports from c-parser are mostly declared in c-tree.h but I think having a c-parser.h for all the new exported stuff is cleaner. I'd wish one of the C frontend maintainers would have a quick look at the overall structure and guide us here - they are the ones that have to approve the patch in the end. (CCed) Thanks, Richard. > > Thanks, > Prasad > >> 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
