Re: [gimplefe] hacking pass manager

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

On 20 July 2016 at 11:34, Richard Biener <richard.guenther@gmail.com> wrote:
> On Tue, Jul 19, 2016 at 10:09 PM, Prasad Ghangal
> <prasad.ghangal@gmail.com> wrote:
>> On 19 July 2016 at 11:04, Richard Biener <richard.guenther@gmail.com> wrote:
>>> On July 18, 2016 11:05:58 PM GMT+02:00, David Malcolm <dmalcolm@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.guenther@gmail.com>
>>>>> wrote:
>>>>> > On July 18, 2016 8:28:15 PM GMT+02:00, Prasad Ghangal <
>>>>> > prasad.ghangal@gmail.com> wrote:
>>>>> > > On 15 July 2016 at 16:13, Richard Biener <
>>>>> > > richard.guenther@gmail.com>
>>>>> > > wrote:
>>>>> > > > On Sun, Jul 10, 2016 at 6:13 PM, Prasad Ghangal
>>>>> > > > <prasad.ghangal@gmail.com> wrote:
>>>>> > > > > On 8 July 2016 at 13:13, Richard Biener <
>>>>> > > > > richard.guenther@gmail.com>
>>>>> > > wrote:
>>>>> > > > > > On Thu, Jul 7, 2016 at 9:45 PM, Prasad Ghangal
>>>>> > > <prasad.ghangal@gmail.com> wrote:
>>>>> > > > > > > On 6 July 2016 at 14:24, Richard Biener
>>>>> > > <richard.guenther@gmail.com> wrote:
>>>>> > > > > > > > On Wed, Jul 6, 2016 at 9:51 AM, Prasad Ghangal
>>>>> > > <prasad.ghangal@gmail.com> wrote:
>>>>> > > > > > > > > On 30 June 2016 at 17:10, Richard Biener
>>>>> > > <richard.guenther@gmail.com> wrote:
>>>>> > > > > > > > > > On Wed, Jun 29, 2016 at 9:13 PM, Prasad Ghangal
>>>>> > > > > > > > > > <prasad.ghangal@gmail.com> wrote:
>>>>> > > > > > > > > > > On 29 June 2016 at 22:15, Richard Biener
>>>>> > > <richard.guenther@gmail.com> wrote:
>>>>> > > > > > > > > > > > On June 29, 2016 6:20:29 PM GMT+02:00,
>>>>> > > > > > > > > > > > Prathamesh Kulkarni
>>>>> > > <prathamesh.kulkarni@linaro.org> wrote:
>>>>> > > > > > > > > > > > > On 18 June 2016 at 12:02, Prasad Ghangal
>>>>> > > <prasad.ghangal@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 ?

Thanks,
Prathamesh
>
> Richard.
>
>>
>>> Richard.
>>>
>>>>Dave
>>>
>>>