On 26 August 2016 at 14:28, Richard Biener <richard.guent...@gmail.com> wrote: > 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. >
PING. https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01837.html Thanks, Prasad >> >> 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
