[patch] [gsoc] [gimplefe] GIMPLE FE Project

Prasad Ghangal prasad.ghangal@gmail.com
Tue Oct 25 11:13:00 GMT 2016


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



More information about the Gcc-patches mailing list