On 14 September 2016 at 18:54, Richard Biener <richard.guent...@gmail.com> wrote: > On Fri, Sep 9, 2016 at 12:38 PM, Prasad Ghangal > <prasad.ghan...@gmail.com> wrote: >> 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 > > (that was a ping for the C FE maintainers) > > Prasad, can you update the git branch with the changes from the last patch you > sent out? I don't think you need to keep it unchanged from this point on. > I have updated the git branch: https://github.com/PrasadG193/gcc_gimple_fe
Thanks, Prasad > Thanks, > Richard. > >> >> 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
