From: Richard Biener <richard.guenther@gmail.com>
To: Prasad Ghangal <prasad.ghangal@gmail.com>
Cc: Joseph Myers <joseph@codesourcery.com>,
Trevor Saunders <tbsaunde@tbsaunde.org>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [patch] [gsoc] [gimplefe] GIMPLE FE Project
Date: Tue, 25 Oct 2016 11:20:00 -0000 [thread overview]
Message-ID: <CAFiYyc1eA86jvzMeqw0WXh_vhbeM-1n6L6BfJzPEnr0Rzw0BUA@mail.gmail.com> (raw)
In-Reply-To: <CAE+uiWZV5+WedneOVCE6jHL4gBhOLWeGVfxLGdUuOMU5ti1WWg@mail.gmail.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
prev parent reply other threads:[~2016-10-25 11:20 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-21 17:05 Prasad Ghangal
2016-08-22 11:17 ` Trevor Saunders
2016-08-22 18:40 ` Prasad Ghangal
2016-08-22 21:20 ` Trevor Saunders
2016-08-22 21:27 ` David Malcolm
2016-08-22 21:54 ` Pedro Alves
2016-08-23 7:47 ` Richard Biener
2016-08-23 10:32 ` Pedro Alves
2016-08-23 15:17 ` Trevor Saunders
2016-08-23 15:45 ` Prasad Ghangal
2016-08-24 7:03 ` Prasad Ghangal
2016-08-24 8:45 ` Richard Biener
2016-08-24 10:02 ` Richard Biener
2016-08-26 3:10 ` Prasad Ghangal
2016-08-26 8:58 ` Richard Biener
2016-09-09 10:58 ` Prasad Ghangal
2016-09-14 13:28 ` Richard Biener
2016-09-14 13:37 ` Marek Polacek
2016-09-14 13:38 ` Richard Biener
2016-09-15 4:23 ` Prasad Ghangal
2016-10-05 23:28 ` Joseph Myers
2016-10-25 10:19 ` Richard Biener
2016-10-25 11:13 ` Prasad Ghangal
2016-10-25 11:20 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc1eA86jvzMeqw0WXh_vhbeM-1n6L6BfJzPEnr0Rzw0BUA@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=joseph@codesourcery.com \
--cc=prasad.ghangal@gmail.com \
--cc=tbsaunde@tbsaunde.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).