| Message ID | CAE+uiWY1-vacUBMXiPM28S6b-0VO70t8CoySVK82PCmEuZV8GA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 26, 2016 at 5:08 AM, Prasad Ghangal <prasad.ghangal@gmail.com> wrote: > On 24 August 2016 at 15:32, Richard Biener <richard.guenther@gmail.com> wrote: >> On Mon, Aug 22, 2016 at 8:40 PM, Prasad Ghangal >> <prasad.ghangal@gmail.com> wrote: >>> On 22 August 2016 at 16:55, Trevor Saunders <tbsaunde@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
On 26 August 2016 at 14:28, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Aug 26, 2016 at 5:08 AM, Prasad Ghangal > <prasad.ghangal@gmail.com> wrote: >> On 24 August 2016 at 15:32, Richard Biener <richard.guenther@gmail.com> wrote: >>> On Mon, Aug 22, 2016 at 8:40 PM, Prasad Ghangal >>> <prasad.ghangal@gmail.com> wrote: >>>> On 22 August 2016 at 16:55, Trevor Saunders <tbsaunde@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
On Fri, Sep 9, 2016 at 12:38 PM, Prasad Ghangal <prasad.ghangal@gmail.com> wrote: > On 26 August 2016 at 14:28, Richard Biener <richard.guenther@gmail.com> wrote: >> On Fri, Aug 26, 2016 at 5:08 AM, Prasad Ghangal >> <prasad.ghangal@gmail.com> wrote: >>> On 24 August 2016 at 15:32, Richard Biener <richard.guenther@gmail.com> wrote: >>>> On Mon, Aug 22, 2016 at 8:40 PM, Prasad Ghangal >>>> <prasad.ghangal@gmail.com> wrote: >>>>> On 22 August 2016 at 16:55, Trevor Saunders <tbsaunde@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. 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
On Wed, Sep 14, 2016 at 03:24:18PM +0200, Richard Biener wrote: > > 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. So where exactly is the latest patch/branch? I see the gimple-front-end branch, but that hasn't been updated for 2 years. Marek
On Wed, Sep 14, 2016 at 3:28 PM, Marek Polacek <polacek@redhat.com> wrote: > On Wed, Sep 14, 2016 at 03:24:18PM +0200, Richard Biener wrote: >> > 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. > > So where exactly is the latest patch/branch? I see the gimple-front-end > branch, but that hasn't been updated for 2 years. Latest patch was posted here: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01830.html and the git (not up to date with that patch) is at https://github.com/PrasadG193/gcc_gimple_fe.git Richard. > Marek
On 14 September 2016 at 18:54, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Sep 9, 2016 at 12:38 PM, Prasad Ghangal > <prasad.ghangal@gmail.com> wrote: >> On 26 August 2016 at 14:28, Richard Biener <richard.guenther@gmail.com> wrote: >>> On Fri, Aug 26, 2016 at 5:08 AM, Prasad Ghangal >>> <prasad.ghangal@gmail.com> wrote: >>>> On 24 August 2016 at 15:32, Richard Biener <richard.guenther@gmail.com> wrote: >>>>> On Mon, Aug 22, 2016 at 8:40 PM, Prasad Ghangal >>>>> <prasad.ghangal@gmail.com> wrote: >>>>>> On 22 August 2016 at 16:55, Trevor Saunders <tbsaunde@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
On Fri, 26 Aug 2016, Prasad Ghangal wrote: > >> 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 ? I think the GIMPLE parsing should go in a separate file (meaning exporting relevant types and function declarations in a new c-parser.h). > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index a5358ed..3c4d2cc 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -200,6 +200,10 @@ F > Driver C ObjC C++ ObjC++ Joined Separate MissingArgError(missing path after %qs) > -F <dir> Add <dir> to the end of the main framework include path. > > +fgimple > +C Var(flag_gimple) Init(0) > +Enable parsing GIMPLE You should get a test failure here from the missing "." at the end of the help text. Of course the option also needs documenting in invoke.texi. > @@ -1659,6 +1695,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok, > tree all_prefix_attrs; > bool diagnosed_no_specs = false; > location_t here = c_parser_peek_token (parser)->location; > + bool gimple_body_p = false; > + opt_pass *pass = NULL; > + bool startwith_p = false; The comment above the function needs updating to document the new syntax. > +static void > +c_parser_gimple_expression (c_parser *parser, gimple_seq *seq) > +{ > + struct c_expr lhs, rhs; > + gimple *assign = NULL; > + enum tree_code subcode = NOP_EXPR; > + location_t loc; > + tree arg = NULL_TREE; > + auto_vec<tree> vargs; > + > + lhs = c_parser_gimple_unary_expression (parser); > + rhs.value = error_mark_node; > + > + if (c_parser_next_token_is (parser, CPP_EQ)) > + { > + c_parser_consume_token (parser); > + } Redundant braces around a single statement. Also, this looks wrong, in that it seems like you'd accept a random '=' token at this point regardless of what follows and whether '=' makes sense in this context. You need to have proper cases: if '=' parse what makes sense after '=', otherwise parse what makes sense without '=', so that invalid syntax is not accepted. > + if (c_parser_next_token_is (parser, CPP_AND) || > + c_parser_next_token_is (parser, CPP_MULT) || > + c_parser_next_token_is (parser, CPP_PLUS) || > + c_parser_next_token_is (parser, CPP_MINUS) || > + c_parser_next_token_is (parser, CPP_COMPL) || > + c_parser_next_token_is (parser, CPP_NOT)) Operators go at the start of a continuation line, not at the end of the line. > + if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) > + { > + return; > + } Please generally review the patch for redundant braces and remove them. > + /* ssa token string. */ > + const char *ssa_token = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value); > + token = new char [strlen (ssa_token)]; > + strcpy (token, ssa_token); That looks like a buffer overrun. To copy a string ssa_token, you need strlen (ssa_token) + 1 bytes of space. > + /* seperate var name and version. */ Uppercase letters at start of comments, throughout the patch (and it should be "Separate", with 'a' not 'e').
On Thu, Oct 6, 2016 at 1:28 AM, Joseph Myers <joseph@codesourcery.com> wrote: > On Fri, 26 Aug 2016, Prasad Ghangal wrote: > >> >> 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 ? Thanks Joseph for the review. Prasad - do you have time in the next few weeks to continue working on this? I'm currently trying to move what is on the github branch to a branch on git://gcc.gnu.org/git/gcc.git to make it mergeable (looks like the github repo isn't a clone of the gcc git mirror on github?). Thanks, Richard. > I think the GIMPLE parsing should go in a separate file (meaning exporting > relevant types and function declarations in a new c-parser.h). > >> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt >> index a5358ed..3c4d2cc 100644 >> --- a/gcc/c-family/c.opt >> +++ b/gcc/c-family/c.opt >> @@ -200,6 +200,10 @@ F >> Driver C ObjC C++ ObjC++ Joined Separate MissingArgError(missing path after %qs) >> -F <dir> Add <dir> to the end of the main framework include path. >> >> +fgimple >> +C Var(flag_gimple) Init(0) >> +Enable parsing GIMPLE > > You should get a test failure here from the missing "." at the end of the > help text. > > Of course the option also needs documenting in invoke.texi. > >> @@ -1659,6 +1695,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok, >> tree all_prefix_attrs; >> bool diagnosed_no_specs = false; >> location_t here = c_parser_peek_token (parser)->location; >> + bool gimple_body_p = false; >> + opt_pass *pass = NULL; >> + bool startwith_p = false; > > The comment above the function needs updating to document the new syntax. > >> +static void >> +c_parser_gimple_expression (c_parser *parser, gimple_seq *seq) >> +{ >> + struct c_expr lhs, rhs; >> + gimple *assign = NULL; >> + enum tree_code subcode = NOP_EXPR; >> + location_t loc; >> + tree arg = NULL_TREE; >> + auto_vec<tree> vargs; >> + >> + lhs = c_parser_gimple_unary_expression (parser); >> + rhs.value = error_mark_node; >> + >> + if (c_parser_next_token_is (parser, CPP_EQ)) >> + { >> + c_parser_consume_token (parser); >> + } > > Redundant braces around a single statement. Also, this looks wrong, in > that it seems like you'd accept a random '=' token at this point > regardless of what follows and whether '=' makes sense in this context. > You need to have proper cases: if '=' parse what makes sense after '=', > otherwise parse what makes sense without '=', so that invalid syntax is > not accepted. > >> + if (c_parser_next_token_is (parser, CPP_AND) || >> + c_parser_next_token_is (parser, CPP_MULT) || >> + c_parser_next_token_is (parser, CPP_PLUS) || >> + c_parser_next_token_is (parser, CPP_MINUS) || >> + c_parser_next_token_is (parser, CPP_COMPL) || >> + c_parser_next_token_is (parser, CPP_NOT)) > > Operators go at the start of a continuation line, not at the end of the > line. > >> + if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) >> + { >> + return; >> + } > > Please generally review the patch for redundant braces and remove them. > >> + /* ssa token string. */ >> + const char *ssa_token = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value); >> + token = new char [strlen (ssa_token)]; >> + strcpy (token, ssa_token); > > That looks like a buffer overrun. To copy a string ssa_token, you need > strlen (ssa_token) + 1 bytes of space. > >> + /* seperate var name and version. */ > > Uppercase letters at start of comments, throughout the patch (and it > should be "Separate", with 'a' not 'e'). > > -- > Joseph S. Myers > joseph@codesourcery.com
On 25 October 2016 at 15:49, Richard Biener <richard.guenther@gmail.com> wrote: > On Thu, Oct 6, 2016 at 1:28 AM, Joseph Myers <joseph@codesourcery.com> wrote: >> On Fri, 26 Aug 2016, Prasad Ghangal wrote: >> >>> >> 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 ? > > Thanks Joseph for the review. Prasad - do you have time in the next few weeks > to continue working on this? I'm currently trying to move what is on the github > branch to a branch on git://gcc.gnu.org/git/gcc.git to make it mergeable Sorry I couldn't work on the project in last few weeks. Since I will be on vacation in the next week, I will definitely work on it. If we can't merge it before closure of stage1, can we merge it in the stage2 or stage3? > (looks like the github repo isn't a clone of the gcc git mirror on github?). No. (Unfortunately) I had reinitialised git locally. That's why I am also struggling while rebasing and syncing to the trunk. Any solution? Since I am employed now, do I need to update the copyright assignment? Thanks, Prasad > > Thanks, > Richard. > >> I think the GIMPLE parsing should go in a separate file (meaning exporting >> relevant types and function declarations in a new c-parser.h). >> >>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt >>> index a5358ed..3c4d2cc 100644 >>> --- a/gcc/c-family/c.opt >>> +++ b/gcc/c-family/c.opt >>> @@ -200,6 +200,10 @@ F >>> Driver C ObjC C++ ObjC++ Joined Separate MissingArgError(missing path after %qs) >>> -F <dir> Add <dir> to the end of the main framework include path. >>> >>> +fgimple >>> +C Var(flag_gimple) Init(0) >>> +Enable parsing GIMPLE >> >> You should get a test failure here from the missing "." at the end of the >> help text. >> >> Of course the option also needs documenting in invoke.texi. >> >>> @@ -1659,6 +1695,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok, >>> tree all_prefix_attrs; >>> bool diagnosed_no_specs = false; >>> location_t here = c_parser_peek_token (parser)->location; >>> + bool gimple_body_p = false; >>> + opt_pass *pass = NULL; >>> + bool startwith_p = false; >> >> The comment above the function needs updating to document the new syntax. >> >>> +static void >>> +c_parser_gimple_expression (c_parser *parser, gimple_seq *seq) >>> +{ >>> + struct c_expr lhs, rhs; >>> + gimple *assign = NULL; >>> + enum tree_code subcode = NOP_EXPR; >>> + location_t loc; >>> + tree arg = NULL_TREE; >>> + auto_vec<tree> vargs; >>> + >>> + lhs = c_parser_gimple_unary_expression (parser); >>> + rhs.value = error_mark_node; >>> + >>> + if (c_parser_next_token_is (parser, CPP_EQ)) >>> + { >>> + c_parser_consume_token (parser); >>> + } >> >> Redundant braces around a single statement. Also, this looks wrong, in >> that it seems like you'd accept a random '=' token at this point >> regardless of what follows and whether '=' makes sense in this context. >> You need to have proper cases: if '=' parse what makes sense after '=', >> otherwise parse what makes sense without '=', so that invalid syntax is >> not accepted. >> >>> + if (c_parser_next_token_is (parser, CPP_AND) || >>> + c_parser_next_token_is (parser, CPP_MULT) || >>> + c_parser_next_token_is (parser, CPP_PLUS) || >>> + c_parser_next_token_is (parser, CPP_MINUS) || >>> + c_parser_next_token_is (parser, CPP_COMPL) || >>> + c_parser_next_token_is (parser, CPP_NOT)) >> >> Operators go at the start of a continuation line, not at the end of the >> line. >> >>> + if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) >>> + { >>> + return; >>> + } >> >> Please generally review the patch for redundant braces and remove them. >> >>> + /* ssa token string. */ >>> + const char *ssa_token = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value); >>> + token = new char [strlen (ssa_token)]; >>> + strcpy (token, ssa_token); >> >> That looks like a buffer overrun. To copy a string ssa_token, you need >> strlen (ssa_token) + 1 bytes of space. >> >>> + /* seperate var name and version. */ >> >> Uppercase letters at start of comments, throughout the patch (and it >> should be "Separate", with 'a' not 'e'). >> >> -- >> Joseph S. Myers >> joseph@codesourcery.com
On Tue, Oct 25, 2016 at 1:13 PM, Prasad Ghangal <prasad.ghangal@gmail.com> wrote: > On 25 October 2016 at 15:49, Richard Biener <richard.guenther@gmail.com> wrote: >> On Thu, Oct 6, 2016 at 1:28 AM, Joseph Myers <joseph@codesourcery.com> wrote: >>> On Fri, 26 Aug 2016, Prasad Ghangal wrote: >>> >>>> >> 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 ? >> >> Thanks Joseph for the review. Prasad - do you have time in the next few weeks >> to continue working on this? I'm currently trying to move what is on the github >> branch to a branch on git://gcc.gnu.org/git/gcc.git to make it mergeable > Sorry I couldn't work on the project in last few weeks. Since I will > be on vacation in the next week, I will definitely work on it. If we > can't merge it before closure of stage1, can we merge it in the stage2 > or stage3? I think we might be able to merge early during stage3 as well. I'm working my way through the changes that affect not just the GIMPLE FE itself. >> (looks like the github repo isn't a clone of the gcc git mirror on github?). > No. (Unfortunately) I had reinitialised git locally. That's why I am > also struggling while rebasing and syncing to the trunk. Any solution? I almost finished pushing the last state plus Josephs review comments fixed (the style ones) to the official GCC git mirror as a branch off current trunk. I'll send a short announcement once I managed to "push" ... > Since I am employed now, do I need to update the copyright assignment? If your employer has a copyright assignment then things should be fine. If you work on this during non-work time then your personal assignment is also fine. Thanks, Richard. > > Thanks, > Prasad >> >> Thanks, >> Richard. >> >>> I think the GIMPLE parsing should go in a separate file (meaning exporting >>> relevant types and function declarations in a new c-parser.h). >>> >>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt >>>> index a5358ed..3c4d2cc 100644 >>>> --- a/gcc/c-family/c.opt >>>> +++ b/gcc/c-family/c.opt >>>> @@ -200,6 +200,10 @@ F >>>> Driver C ObjC C++ ObjC++ Joined Separate MissingArgError(missing path after %qs) >>>> -F <dir> Add <dir> to the end of the main framework include path. >>>> >>>> +fgimple >>>> +C Var(flag_gimple) Init(0) >>>> +Enable parsing GIMPLE >>> >>> You should get a test failure here from the missing "." at the end of the >>> help text. >>> >>> Of course the option also needs documenting in invoke.texi. >>> >>>> @@ -1659,6 +1695,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok, >>>> tree all_prefix_attrs; >>>> bool diagnosed_no_specs = false; >>>> location_t here = c_parser_peek_token (parser)->location; >>>> + bool gimple_body_p = false; >>>> + opt_pass *pass = NULL; >>>> + bool startwith_p = false; >>> >>> The comment above the function needs updating to document the new syntax. >>> >>>> +static void >>>> +c_parser_gimple_expression (c_parser *parser, gimple_seq *seq) >>>> +{ >>>> + struct c_expr lhs, rhs; >>>> + gimple *assign = NULL; >>>> + enum tree_code subcode = NOP_EXPR; >>>> + location_t loc; >>>> + tree arg = NULL_TREE; >>>> + auto_vec<tree> vargs; >>>> + >>>> + lhs = c_parser_gimple_unary_expression (parser); >>>> + rhs.value = error_mark_node; >>>> + >>>> + if (c_parser_next_token_is (parser, CPP_EQ)) >>>> + { >>>> + c_parser_consume_token (parser); >>>> + } >>> >>> Redundant braces around a single statement. Also, this looks wrong, in >>> that it seems like you'd accept a random '=' token at this point >>> regardless of what follows and whether '=' makes sense in this context. >>> You need to have proper cases: if '=' parse what makes sense after '=', >>> otherwise parse what makes sense without '=', so that invalid syntax is >>> not accepted. >>> >>>> + if (c_parser_next_token_is (parser, CPP_AND) || >>>> + c_parser_next_token_is (parser, CPP_MULT) || >>>> + c_parser_next_token_is (parser, CPP_PLUS) || >>>> + c_parser_next_token_is (parser, CPP_MINUS) || >>>> + c_parser_next_token_is (parser, CPP_COMPL) || >>>> + c_parser_next_token_is (parser, CPP_NOT)) >>> >>> Operators go at the start of a continuation line, not at the end of the >>> line. >>> >>>> + if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>")) >>>> + { >>>> + return; >>>> + } >>> >>> Please generally review the patch for redundant braces and remove them. >>> >>>> + /* ssa token string. */ >>>> + const char *ssa_token = IDENTIFIER_POINTER (c_parser_peek_token (parser)->value); >>>> + token = new char [strlen (ssa_token)]; >>>> + strcpy (token, ssa_token); >>> >>> That looks like a buffer overrun. To copy a string ssa_token, you need >>> strlen (ssa_token) + 1 bytes of space. >>> >>>> + /* seperate var name and version. */ >>> >>> Uppercase letters at start of comments, throughout the patch (and it >>> should be "Separate", with 'a' not 'e'). >>> >>> -- >>> Joseph S. Myers >>> joseph@codesourcery.com
diff --git a/gcc/testsuite/gcc.dg/gimplefe-1.c b/gcc/testsuite/gcc.dg/gimplefe-1.c new file mode 100644 index 0000000..0786d47 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-1.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +int i; +void __GIMPLE () foo() +{ + i = 1; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-10.c b/gcc/testsuite/gcc.dg/gimplefe-10.c new file mode 100644 index 0000000..7f63c58 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-10.c @@ -0,0 +1,18 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +int __GIMPLE() bar(int a, int b, int c) +{ + a = 1; + b = a + 1; + c = b * 4; + return b; +} + +void __GIMPLE() foo() +{ + int a; + int b; + int c; + b = bar(a, b, c); +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-11.c b/gcc/testsuite/gcc.dg/gimplefe-11.c new file mode 100644 index 0000000..e1483f4 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-11.c @@ -0,0 +1,18 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE() bar(int a, int b, int c) +{ + a = 1; + b = a + 1; + c = b * 4; + return; +} + +void __GIMPLE() foo() +{ + int a; + int b; + int c; + bar(a, b, c); +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-12.c b/gcc/testsuite/gcc.dg/gimplefe-12.c new file mode 100644 index 0000000..cbaf8a6 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-12.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE (startwith ("tree-ccp1")) foo () +{ + int a; + int b; + a = b + 2; + return; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-13.c b/gcc/testsuite/gcc.dg/gimplefe-13.c new file mode 100644 index 0000000..c0da9fa --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-13.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE (startwith ("tree-dse1")) foo () +{ + int a; + +bb_2: + if (a > 4) + goto bb_3; + else + goto bb_4; + +bb_3: + a_2 = 10; + goto bb_5; + +bb_4: + a_3 = 20; + +bb_5: + a_1 = __PHI (bb_3: a_2, bb_4: a_3); + a_4 = a_1 + 4; + +return; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-2.c b/gcc/testsuite/gcc.dg/gimplefe-2.c new file mode 100644 index 0000000..e3a23cf --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-2.c @@ -0,0 +1,11 @@ +/* { dg-do compile }*/ +/* { dg-options "-fgimple" } */ + +int a; +void __GIMPLE () foo () +{ + int b; + b = a; + b = b + 1; + a = b; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-3.c b/gcc/testsuite/gcc.dg/gimplefe-3.c new file mode 100644 index 0000000..595365e --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-3.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE () foo () +{ + int *b; + *b = 1; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-4.c b/gcc/testsuite/gcc.dg/gimplefe-4.c new file mode 100644 index 0000000..3600c7c --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-4.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE () foo () +{ + int a; + char b; + a = (int) b; + return; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-5.c b/gcc/testsuite/gcc.dg/gimplefe-5.c new file mode 100644 index 0000000..1dab4af --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-5.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +int a; +void __GIMPLE () foo () +{ + int b; + int c; + +bb_2: + b = a; + if (b > 3) + goto bb_3; + else + goto bb_4; + +bb_3: + b = c + 4; + goto bb_5; + +bb_4: + b = b + 1; + goto bb_5; + +bb_5: + a = b; + return; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-6.c b/gcc/testsuite/gcc.dg/gimplefe-6.c new file mode 100644 index 0000000..242e08f --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-6.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE () foo () +{ + int a; + int b; + int c; + int d; + +bb_2: + a = ~b; + b = a << c; + c = a & b; + d = b | c; + +bb_3: + return; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-7.c b/gcc/testsuite/gcc.dg/gimplefe-7.c new file mode 100644 index 0000000..6125541 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-7.c @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +void __GIMPLE () foo () +{ + int a; + +bb_2: + if (a > 4) + goto bb_3; + else + goto bb_4; + +bb_3: + a_2 = 10; + goto bb_5; + +bb_4: + a_3 = 20; + +bb_5: + a_1 = __PHI (bb_3: a_2, bb_4: a_3); + a_4 = a_1 + 4; + +return; +} + diff --git a/gcc/testsuite/gcc.dg/gimplefe-8.c b/gcc/testsuite/gcc.dg/gimplefe-8.c new file mode 100644 index 0000000..4936bec --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-8.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +int __GIMPLE () foo () +{ + int a; + int b; + +bb_2: + b = a_1(D) + 1; +bb_3: + return b; +} diff --git a/gcc/testsuite/gcc.dg/gimplefe-9.c b/gcc/testsuite/gcc.dg/gimplefe-9.c new file mode 100644 index 0000000..a24be273 --- /dev/null +++ b/gcc/testsuite/gcc.dg/gimplefe-9.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-fgimple" } */ + +int __GIMPLE() bar() +{ + int a; + a = a + 1; + return a; +} + +void __GIMPLE() foo() +{ + int b; + b = bar(); +}