[patch] [gsoc] [gimplefe] GIMPLE FE Project
Prasad Ghangal
prasad.ghangal@gmail.com
Thu Sep 15 04:23:00 GMT 2016
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
More information about the Gcc-patches
mailing list