On Wed, Jul 20, 2016 at 1:46 PM, Prathamesh Kulkarni
<prathamesh.kulka...@linaro.org> wrote:
> On 20 July 2016 at 11:34, Richard Biener <richard.guent...@gmail.com> wrote:
>> On Tue, Jul 19, 2016 at 10:09 PM, Prasad Ghangal
>> <prasad.ghan...@gmail.com> wrote:
>>> On 19 July 2016 at 11:04, Richard Biener <richard.guent...@gmail.com> wrote:
>>>> On July 18, 2016 11:05:58 PM GMT+02:00, David Malcolm
>>>> <dmalc...@redhat.com> wrote:
>>>>>On Tue, 2016-07-19 at 00:52 +0530, Prasad Ghangal wrote:
>>>>>> On 19 July 2016 at 00:25, Richard Biener <richard.guent...@gmail.com>
>>>>>> wrote:
>>>>>> > On July 18, 2016 8:28:15 PM GMT+02:00, Prasad Ghangal <
>>>>>> > prasad.ghan...@gmail.com> wrote:
>>>>>> > > On 15 July 2016 at 16:13, Richard Biener <
>>>>>> > > richard.guent...@gmail.com>
>>>>>> > > wrote:
>>>>>> > > > On Sun, Jul 10, 2016 at 6:13 PM, Prasad Ghangal
>>>>>> > > > <prasad.ghan...@gmail.com> wrote:
>>>>>> > > > > On 8 July 2016 at 13:13, Richard Biener <
>>>>>> > > > > richard.guent...@gmail.com>
>>>>>> > > wrote:
>>>>>> > > > > > On Thu, Jul 7, 2016 at 9:45 PM, Prasad Ghangal
>>>>>> > > <prasad.ghan...@gmail.com> wrote:
>>>>>> > > > > > > On 6 July 2016 at 14:24, Richard Biener
>>>>>> > > <richard.guent...@gmail.com> wrote:
>>>>>> > > > > > > > On Wed, Jul 6, 2016 at 9:51 AM, Prasad Ghangal
>>>>>> > > <prasad.ghan...@gmail.com> wrote:
>>>>>> > > > > > > > > On 30 June 2016 at 17:10, Richard Biener
>>>>>> > > <richard.guent...@gmail.com> wrote:
>>>>>> > > > > > > > > > On Wed, Jun 29, 2016 at 9:13 PM, Prasad Ghangal
>>>>>> > > > > > > > > > <prasad.ghan...@gmail.com> wrote:
>>>>>> > > > > > > > > > > On 29 June 2016 at 22:15, Richard Biener
>>>>>> > > <richard.guent...@gmail.com> wrote:
>>>>>> > > > > > > > > > > > On June 29, 2016 6:20:29 PM GMT+02:00,
>>>>>> > > > > > > > > > > > Prathamesh Kulkarni
>>>>>> > > <prathamesh.kulka...@linaro.org> wrote:
>>>>>> > > > > > > > > > > > > On 18 June 2016 at 12:02, Prasad Ghangal
>>>>>> > > <prasad.ghan...@gmail.com>
>>>>>> > > > > > > > > > > > > wrote:
>>>>>> > > > > > > > > > > > > > Hi,
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > I tried hacking pass manager to execute
>>>>>> > > > > > > > > > > > > > only given passes.
>>>>>> > > For this I
>>>>>> > > > > > > > > > > > > > am adding new member as opt_pass
>>>>>> > > > > > > > > > > > > > *custom_pass_list to the
>>>>>> > > function
>>>>>> > > > > > > > > > > > > > structure to store passes need to execute
>>>>>> > > > > > > > > > > > > > and providing the
>>>>>> > > > > > > > > > > > > > custom_pass_list to execute_pass_list()
>>>>>> > > > > > > > > > > > > > function instead of
>>>>>> > > all
>>>>>> > > > > > > > > > > > > passes
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > for test case like-
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > int a;
>>>>>> > > > > > > > > > > > > > void __GIMPLE (execute ("tree-ccp1", "tree
>>>>>> > > > > > > > > > > > > > -fre1")) foo()
>>>>>> > > > > > > > > > > > > > {
>>>>>> > > > > > > > > > > > > > bb_1:
>>>>>> > > > > > > > > > > > > > a = 1 + a;
>>>>>> > > > > > > > > > > > > > }
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > it will execute only given passes i.e. ccp1
>>>>>> > > > > > > > > > > > > > and fre1 pass
>>>>>> > > on the
>>>>>> > > > > > > > > > > > > function
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > and for test case like -
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > int a;
>>>>>> > > > > > > > > > > > > > void __GIMPLE (startwith ("tree-ccp1"))
>>>>>> > > > > > > > > > > > > > foo()
>>>>>> > > > > > > > > > > > > > {
>>>>>> > > > > > > > > > > > > > bb_1:
>>>>>> > > > > > > > > > > > > > a = 1 + a;
>>>>>> > > > > > > > > > > > > > }
>>>>>> > > > > > > > > > > > > >
>>>>>> > > > > > > > > > > > > > it will act as a entry point to the
>>>>>> > > > > > > > > > > > > > pipeline and will
>>>>>> > > execute passes
>>>>>> > > > > > > > > > > > > > starting from given pass.
>>>>>> > > > > > > > > > > > > Bike-shedding:
>>>>>> > > > > > > > > > > > > Would it make sense to have syntax for
>>>>>> > > > > > > > > > > > > defining pass ranges
>>>>>> > > to execute
>>>>>> > > > > > > > > > > > > ?
>>>>>> > > > > > > > > > > > > for instance:
>>>>>> > > > > > > > > > > > > void __GIMPLE(execute (pass_start :
>>>>>> > > > > > > > > > > > > pass_end))
>>>>>> > > > > > > > > > > > > which would execute all the passes within
>>>>>> > > > > > > > > > > > > range [pass_start,
>>>>>> > > pass_end],
>>>>>> > > > > > > > > > > > > which would be convenient if the range is
>>>>>> > > > > > > > > > > > > large.
>>>>>> > > > > > > > > > > >
>>>>>> > > > > > > > > > > > But it would rely on a particular pass
>>>>>> > > > > > > > > > > > pipeline, f.e.
>>>>>> > > pass-start appearing before pass-end.
>>>>>> > > > > > > > > > > >
>>>>>> > > > > > > > > > > > Currently control doesn't work 100% as it only
>>>>>> > > > > > > > > > > > replaces
>>>>>> > > all_optimizations but not lowering passes or early opts, nor IPA
>>>>>> > > opts.
>>>>>> > > > > > > > > > > >
>>>>>> > > > > > > > > > >
>>>>>> > > > > > > > > > > Each pass needs GIMPLE in some specific form. So
>>>>>> > > > > > > > > > > I am letting
>>>>>> > > lowering
>>>>>> > > > > > > > > > > and early opt passes to execute. I think we have
>>>>>> > > > > > > > > > > to execute
>>>>>> > > some
>>>>>> > > > > > > > > > > passes (like cfg) anyway to represent GIMPLE into
>>>>>> > > > > > > > > > > proper form
>>>>>> > > > > > > > > >
>>>>>> > > > > > > > > > Yes, that's true. Note that early opt passes only
>>>>>> > > > > > > > > > optimize but
>>>>>> > > we need
>>>>>> > > > > > > > > > pass_build_ssa_passes at least (for into-SSA). For
>>>>>> > > > > > > > > > proper
>>>>>> > > unit-testing
>>>>>> > > > > > > > > > of GIMPLE passes we do need to guard off early opts
>>>>>> > > > > > > > > > somehow
>>>>>> > > > > > > > > > (I guess a simple if (flag_gimple && cfun
>>>>>> > > > > > > > > > ->custom_pass_list)
>>>>>> > > would do
>>>>>> > > > > > > > > > that).
>>>>>> > > > > > > > > >
>>>>>> > > > > > > > > > Then there is of course the question about IPA
>>>>>> > > > > > > > > > passes which I
>>>>>> > > think is
>>>>>> > > > > > > > > > somewhat harder (one could always disable all IPA
>>>>>> > > > > > > > > > passes
>>>>>> > > manually
>>>>>> > > > > > > > > > via flags of course or finally have a global
>>>>>> > > > > > > > > > -fipa/no-ipa like
>>>>>> > > most
>>>>>> > > > > > > > > > other compilers).
>>>>>> > > > > > > > > >
>>>>>> > > > > > > > > Can we iterate through all ipa passes and do
>>>>>> > > > > > > > > -fdisable-ipa-pass
>>>>>> > > or
>>>>>> > > > > > > > > -fenable-ipa-pass equivalent for each?
>>>>>> > > > > > > >
>>>>>> > > > > > > > We could do that, yes. But let's postpone this issue.
>>>>>> > > > > > > > I think
>>>>>> > > that
>>>>>> > > > > > > > startwith is going to be most useful and rather than
>>>>>> > > > > > > > constructing
>>>>>> > > > > > > > a pass list for it "native" support for it in the pass
>>>>>> > > > > > > > manager is
>>>>>> > > > > > > > likely to produce better results (add a 'startwith'
>>>>>> > > > > > > > member
>>>>>> > > alongside
>>>>>> > > > > > > > the pass list member and if it is set the pass manager
>>>>>> > > > > > > > skips all
>>>>>> > > > > > > > passes that do not match 'startwith' and once it
>>>>>> > > > > > > > reaches it it
>>>>>> > > clears
>>>>>> > > > > > > > the field).
>>>>>> > > > > > > >
>>>>>> > > > > > > > In the future I hope we can get away from a static pass
>>>>>> > > > > > > > list and
>>>>>> > > more
>>>>>> > > > > > > > towards rule-driven pass execution (we have all those
>>>>>> > > > > > > > PROP_*
>>>>>> > > stuff
>>>>>> > > > > > > > already but it isn't really used for example). But
>>>>>> > > > > > > > well, that
>>>>>> > > would be
>>>>>> > > > > > > > a separate GSoC project ;)
>>>>>> > > > > > > >
>>>>>> > > > > > > > IMHO startwith will provide everything needed for unit
>>>>>> > > > > > > > -testing.
>>>>>> > > We can
>>>>>> > > > > > > > add a flag on whether further passes should be executed
>>>>>> > > > > > > > or not
>>>>>> > > and
>>>>>> > > > > > > > even a pass list like execute ("ccp1", "fre") can be
>>>>>> > > > > > > > implemented
>>>>>> > > by
>>>>>> > > > > > > > startwith ccp1 and then from there executing the rest
>>>>>> > > > > > > > of the
>>>>>> > > passes in the
>>>>>> > > > > > > > list and stopping at the end.
>>>>>> > > > > > > >
>>>>>> > > > > > > > As said, unit-testing should exercise a single pass if
>>>>>> > > > > > > > we can
>>>>>> > > control
>>>>>> > > > > > > > its input.
>>>>>> > > > > > > >
>>>>>> > > > > > > In this patch I am skipping execution of passes until
>>>>>> > > pass_startwith
>>>>>> > > > > > > is found. Unlike previous build, now pass manager
>>>>>> > > > > > > executes all
>>>>>> > > passes
>>>>>> > > > > > > in pipeline starting from pass_startwith instead of just
>>>>>> > > > > > > sub
>>>>>> > > passes.
>>>>>> > > > > >
>>>>>> > > > > > That looks good. I wonder if
>>>>>> > > > > >
>>>>>> > > > > > + if (startwith_p && cfun->startwith)
>>>>>> > > > > > + {
>>>>>> > > > > > + if (pass->name == cfun->pass_startwith->name
>>>>>> > > > > > + || pass->name == "*clean_state")
>>>>>> > > > > >
>>>>>> > > > > > need better be strcmp ()s though. Also the early
>>>>>> > > > > > optimization
>>>>>> > > pipeline
>>>>>> > > > > > should be executed with startwith support as well.
>>>>>> > > > > >
>>>>>> > > > >
>>>>>> > > > > This patch adds startwith support for early opt passes. But
>>>>>> > > > > for
>>>>>> > > > > starting from some passes (like asan0, optimized) in
>>>>>> > > > > all_passes
>>>>>> > > > > pipeline, it falils at verify_curr_properties in
>>>>>> > > > > execute_one_pass
>>>>>> > > ().
>>>>>> > > > > I wonder if we need to update properties after skipping each
>>>>>> > > > > pass
>>>>>> > > >
>>>>>> > > > Yeah, it's not possible to start at arbitrary points with
>>>>>> > > > skipping
>>>>>> > > passes
>>>>>> > > > that provide a required property. I suspect it's good enough
>>>>>> > > > that
>>>>>> > > we'll
>>>>>> > > > ICE if that happens.
>>>>>> > > >
>>>>>> > > > I see you are working on the dump-file side a bit now. What is
>>>>>> > > > still
>>>>>> > > > missing after you got support for PHIs is parsing of SSA names.
>>>>>> > > > Without this unit-testing will be difficult at best.
>>>>>> > > >
>>>>>> > > > I think what we need to do is simplify the job of the parser
>>>>>> > > > and
>>>>>> > > > make the syntax we use to print SSA names a bit different.
>>>>>> > > > So rather than printing VAR_VERSION we need to choose a
>>>>>> > > > letter that is not part of a valid identifier before VERSION,
>>>>>> > > > like a dot '.'. Thus we'd have i.1 instead of i_1 and we'd
>>>>>> > > > have
>>>>>> > > > .2 instead of _2 for an anonymous SSA name. The advantage
>>>>>> > > > for non-anonymous names is that we can properly re-use the
>>>>>> > > > C frontends decl handling for declaring and looking up 'i'.
>>>>>> > > > The disadvantage is that for anonymous SSA names this isn't
>>>>>> > > > so easy which means we could choose to not support those
>>>>>> > > > for the moment and require fake decls for them. In fact
>>>>>> > > > into-SSA will do the correct thing if we just treat them as
>>>>>> > > > decls,
>>>>>> > > > thus continue to dump them as _VERSION.
>>>>>> > > >
>>>>>> > >
>>>>>> > > I am little confused here about parsing 'i.1' because lexer drops
>>>>>> > > DOT
>>>>>> > > token for syntax like NAME.NUMBER . Hence instead of 'i.1' parser
>>>>>> > > receives 'i1'
>>>>>> >
>>>>>> > Are you sure? You should get three tokens, one for 'i', one for the
>>>>>> > dot and
>>>>>> > One for '1'. You'd lookup the first via the C frontend symbol
>>>>>> > table only.
>>>>>> >
>>>>>>
>>>>>> Yes, I was also expecting that. For syntax like 'a.b' (ie name.name)
>>>>>> it gives proper 3 tokens but for syntax like 'a.1' it produces only
>>>>>> 2.
>>>>>>
>>>>>> This is what I observed while debugging "int a.1;"
>>>>>>
>>>>>> (gdb) print *c_parser_peek_nth_token (parser, 1)
>>>>>> $3 = {type = CPP_KEYWORD, id_kind = C_ID_NONE, keyword = RID_INT,
>>>>>> pragma_kind = PRAGMA_NONE, location = 242114, value =
>>>>>> 0x7ffff65c82d0}
>>>>>> (gdb) print *c_parser_peek_nth_token (parser, 2)
>>>>>> $4 = {type = CPP_NAME, id_kind = C_ID_ID, keyword = RID_MAX,
>>>>>> pragma_kind = PRAGMA_NONE, location = 242240, value =
>>>>>> 0x7ffff66d0b90}
>>>>>> (gdb) print *c_parser_peek_nth_token (parser, 3)
>>>>>> $5 = {type = CPP_NUMBER, id_kind = C_ID_NONE, keyword = RID_MAX,
>>>>>> pragma_kind = PRAGMA_NONE, location = 242273, value =
>>>>>> 0x7ffff66e0030}
>>>>>
>>>>>What is the number? I'm wondering if it's somehow been lexed as a
>>>>>decimal ".1" i.e. if the "." is somehow being treated as part of the
>>>>>CPP_NUMBER.
>>>>
>>>
>>> Yes, the token '.1' is treated as CPP_NUMBER
>>>
>>>> Ah, possible...
>>>>
>>>>>FWIW, I find hacking in calls to "inform" very useful for seeing what
>>>>>each token is (assuming that caret printing isn't disabled).
>>>>>
>>>>> (gdb) call inform ($3->location, "")
>>>>> (gdb) call inform ($4->location, "")
>>>>> (gdb) call inform ($5
>>>>>->location, "")
>>>>>
>>>>>etc
>>>>>
>>>>>BTW, does it have to be '.' as the SSA_NAME separator? Could it be a
>>>>>different character e.g. '@' or something else that's non-legal in C?
>>>>
>>>> It doesn't have to be '.', but sth visually not too disturbing would be
>>>> nice. If we don't go with '.' We can as well try to parse the SSA version
>>>> from i_1, leaving the ambiguity with it being a valid C identifier.
>>>>
>>>
>>> As special characters are not valid in C, I am using 'a..1' as syntax
>>> for ssa name. For parsing I am using
>>> + lhs.value = make_ssa_name_fn (cfun,
>>> + lookup_name (c_parser_peek_token
>>> (parser)->value),
>>> + NULL);
>>> Now for passing ssa name into __PHI, how can we lookup for particular ssa
>>> name.
>>> Or is there other better method to do it?
>>
>> Note that with this you need to preserve SSA versions as used in the source
>> (the 1 in a..1). If you can do that (see below) the way to lookup an SSA
>> name
>> is simply calling 'ssa_name (version)' with version being an integer with the
>> version number.
>>
>> make_ssa_name will simply allocate the next available SSA version so
>> you'll need to add an interface that allows you allocation of a specific
>> SSA version. Like with sth similar to
>>
>> Index: gcc/tree-ssanames.c
>> ===================================================================
>> --- gcc/tree-ssanames.c (revision 238512)
>> +++ gcc/tree-ssanames.c (working copy)
>> @@ -255,7 +255,7 @@
>> used without a preceding definition). */
>>
>> tree
>> -make_ssa_name_fn (struct function *fn, tree var, gimple *stmt)
>> +make_ssa_name_fn (struct function *fn, tree var, gimple *stmt, int version)
>> {
>> tree t;
>> use_operand_p imm;
>> @@ -265,8 +265,17 @@
>> || TREE_CODE (var) == RESULT_DECL
>> || (TYPE_P (var) && is_gimple_reg_type (var)));
>>
>> + if (version != 0)
>> + {
>> + t = make_node (SSA_NAME);
>> + SSA_NAME_VERSION (t) = version;
>> + vec_safe_grow_cleared (SSANAMES (fn), version + 1);
>> + gcc_assert ((*SSANAMES (fn))[version] == NULL);
>> + (*SSANAMES (fn))[version] = t;
>> + ssa_name_nodes_created++;
>> + }
>> /* If our free list has an element, then use it. */
>> - if (!vec_safe_is_empty (FREE_SSANAMES (fn)))
>> + else if (!vec_safe_is_empty (FREE_SSANAMES (fn)))
>> {
>> t = FREE_SSANAMES (fn)->pop ();
>> ssa_name_nodes_reused++;
>> Index: gcc/tree-ssanames.h
>> ===================================================================
>> --- gcc/tree-ssanames.h (revision 237253)
>> +++ gcc/tree-ssanames.h (working copy)
>> @@ -79,7 +79,7 @@
>> extern void init_ssanames (struct function *, int);
>> extern void fini_ssanames (struct function *);
>> extern void ssanames_print_statistics (void);
>> -extern tree make_ssa_name_fn (struct function *, tree, gimple *);
>> +extern tree make_ssa_name_fn (struct function *, tree, gimple *, int);
>> extern void release_ssa_name_fn (struct function *, tree);
>> extern bool get_ptr_info_alignment (struct ptr_info_def *, unsigned int *,
>> unsigned int *);
>>
>> where you can then do sth like
>>
>> ssaname = ssa_name (version);
>> if (!ssaname)
>> ssaname = make_ssa_name_fn (cfun, lookup_name (...), NULL, version);
>>
>> to either lookup an existing or allocate a new SSA name of the desired
>> version.
>>
>> Just for some bike-shedding, I don't like using '..' too much ;)
> Would it be a good idea to modify libcpp's lexer to recognize
> identifier.number as a single token if -fgimple is enabled ?
I don't think so. As said, we can retain the i_2 syntax as well where you then
get a single token and to decide whether it is a SSA name do a lookup for
'i' first and if that succeeds, interpret _2 as a version suffix,
otherwise use it
as full variable name. Re-constructing the 1 from .1 or 17 from .17 CPP_NUMBER
is possible as well of course.
Richard.
> Thanks,
> Prathamesh
>>
>> Richard.
>>
>>>
>>>> Richard.
>>>>
>>>>>Dave
>>>>
>>>>
