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. 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
