rfa: remove get_var_ann (was: Fix PR50260)

Hi,

On Fri, 2 Sep 2011, Michael Matz wrote:

> > > Ok.  Time to make get_var_ann private?
> 
> As I feared the call to get_var_ann in set_is_used right now really is 
> still needed, privatizing it hence isn't that straight forward.

After pondering I concluded that it's not necessary to call set_is_used 
for variables without var annotation.  Those won't be in referenced_vars 
(or local_decls) and therefore also won't be removed from those lists no 
matter how hard we try.

Regstrapped on x86_64-linux (without Ada).  Okay for trunk?


Ciao,
Michael.

On Sat, Sep 3, 2011 at 3:51 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Fri, 2 Sep 2011, Michael Matz wrote:
>
>> > > Ok.  Time to make get_var_ann private?
>>
>> As I feared the call to get_var_ann in set_is_used right now really is
>> still needed, privatizing it hence isn't that straight forward.
>
> After pondering I concluded that it's not necessary to call set_is_used
> for variables without var annotation.  Those won't be in referenced_vars
> (or local_decls) and therefore also won't be removed from those lists no
> matter how hard we try.
>
> Regstrapped on x86_64-linux (without Ada).  Okay for trunk?

No.  We call mark_all_vars_used on trees contained in GIMPLE
non-debug statements.  All vars we can reach that way _have_
to be in the list of referenced vars.  That they are not is the bug.
So - can you investigate which var doesn't have an annotation
an why it isn't in referenced vars?

Richard.

>
> Ciao,
> Michael.
> --
>        * tree-flow.h (get_var_ann): Don't declare.
>        * tree-flow-inline.h (get_var_ann): Remove.
>        (set_is_used): Use var_ann, not get_var_ann.
>        * tree-dfa.c (add_referenced_var): Inline body of get_var_ann.
>        * tree-ssa-live.c (mark_all_vars_used_1): Check var_ann before
>        calling set_is_used.
>
> Index: tree-flow-inline.h
> ===================================================================
> --- tree-flow-inline.h  (Revision 178488)
> +++ tree-flow-inline.h  (Arbeitskopie)
> @@ -145,16 +145,6 @@ var_ann (const_tree t)
>   return p ? *p : NULL;
>  }
>
> -/* Return the variable annotation for T, which must be a _DECL node.
> -   Create the variable annotation if it doesn't exist.  */
> -static inline var_ann_t
> -get_var_ann (tree var)
> -{
> -  var_ann_t *p = DECL_VAR_ANN_PTR (var);
> -  gcc_checking_assert (p);
> -  return *p ? *p : create_var_ann (var);
> -}
> -
>  /* Get the number of the next statement uid to be allocated.  */
>  static inline unsigned int
>  gimple_stmt_max_uid (struct function *fn)
> @@ -568,7 +558,7 @@ phi_arg_index_from_use (use_operand_p us
>  static inline void
>  set_is_used (tree var)
>  {
> -  var_ann_t ann = get_var_ann (var);
> +  var_ann_t ann = var_ann (var);
>   ann->used = true;
>  }
>
> Index: tree-flow.h
> ===================================================================
> --- tree-flow.h (Revision 178488)
> +++ tree-flow.h (Arbeitskopie)
> @@ -278,7 +278,6 @@ typedef struct immediate_use_iterator_d
>  typedef struct var_ann_d *var_ann_t;
>
>  static inline var_ann_t var_ann (const_tree);
> -static inline var_ann_t get_var_ann (tree);
>  static inline void update_stmt (gimple);
>  static inline int get_lineno (const_gimple);
>
> Index: tree-dfa.c
> ===================================================================
> --- tree-dfa.c  (Revision 178488)
> +++ tree-dfa.c  (Arbeitskopie)
> @@ -580,8 +580,9 @@ set_default_def (tree var, tree def)
>  bool
>  add_referenced_var (tree var)
>  {
> -  get_var_ann (var);
>   gcc_assert (DECL_P (var));
> +  if (!*DECL_VAR_ANN_PTR (var))
> +    create_var_ann (var);
>
>   /* Insert VAR into the referenced_vars hash table if it isn't present.  */
>   if (referenced_var_check_and_insert (var))
> Index: tree-ssa-live.c
> ===================================================================
> --- tree-ssa-live.c     (Revision 178408)
> +++ tree-ssa-live.c     (Arbeitskopie)
> @@ -376,7 +376,10 @@ mark_all_vars_used_1 (tree *tp, int *wal
>     {
>       if (data != NULL && bitmap_clear_bit ((bitmap) data, DECL_UID (t)))
>        mark_all_vars_used (&DECL_INITIAL (t), data);
> -      set_is_used (t);
> +      /* If something has no annotation it's neither in referenced_vars,
> +         nor in local_decls.  No use in marking it as used.  */
> +      if (var_ann (t))
> +       set_is_used (t);
>     }
>   /* remove_unused_scope_block_p requires information about labels
>      which are not DECL_IGNORED_P to tell if they might be used in the IL.  */
>

Hi,

On Sat, 3 Sep 2011, Richard Guenther wrote:

> >> As I feared the call to get_var_ann in set_is_used right now really 
> >> is still needed, privatizing it hence isn't that straight forward.
> >
> > After pondering I concluded that it's not necessary to call 
> > set_is_used for variables without var annotation.  Those won't be in 
> > referenced_vars (or local_decls) and therefore also won't be removed 
> > from those lists no matter how hard we try.
> >
> > Regstrapped on x86_64-linux (without Ada).  Okay for trunk?
> 
> No.  We call mark_all_vars_used on trees contained in GIMPLE non-debug 
> statements.  All vars we can reach that way _have_ to be in the list of 
> referenced vars.

Sometimes global variables (non-auto vars with context != cfun) can be 
missing from referenced_vars.  In the case where we hit bugs with this 
it's the non-local variables generated for profile counters.  I can add 
the respective add_referenced_var calls for that, but I'm not sure I see 
the point.  That of course comes back to our old discussion for what 
purposes referenced_vars actually is used.  I looked at all users and I 
think that the global counters missing from referenced_vars is harmless.

OTOH it's a nice invariant that can actually be checked for (that all 
reachable vars whatsoever have to be in referenced_vars), so I'm going to 
do that.

> That they are not is the bug. So - can you investigate 
> which var doesn't have an annotation an why it isn't in referenced vars?


Ciao,
Michael.

On Sat, Sep 3, 2011 at 5:31 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Sat, 3 Sep 2011, Richard Guenther wrote:
>
>> >> As I feared the call to get_var_ann in set_is_used right now really
>> >> is still needed, privatizing it hence isn't that straight forward.
>> >
>> > After pondering I concluded that it's not necessary to call
>> > set_is_used for variables without var annotation.  Those won't be in
>> > referenced_vars (or local_decls) and therefore also won't be removed
>> > from those lists no matter how hard we try.
>> >
>> > Regstrapped on x86_64-linux (without Ada).  Okay for trunk?
>>
>> No.  We call mark_all_vars_used on trees contained in GIMPLE non-debug
>> statements.  All vars we can reach that way _have_ to be in the list of
>> referenced vars.
>
> Sometimes global variables (non-auto vars with context != cfun) can be
> missing from referenced_vars.  In the case where we hit bugs with this
> it's the non-local variables generated for profile counters.  I can add
> the respective add_referenced_var calls for that, but I'm not sure I see
> the point.  That of course comes back to our old discussion for what
> purposes referenced_vars actually is used.  I looked at all users and I
> think that the global counters missing from referenced_vars is harmless.
>
> OTOH it's a nice invariant that can actually be checked for (that all
> reachable vars whatsoever have to be in referenced_vars), so I'm going to
> do that.

Yes, until we get rid of referenced_vars (which we still should do at
some point...)
that's the best.  IIRC we have some verification code even, and wonder why it
doesn't trigger.

Richard.

>> That they are not is the bug. So - can you investigate
>> which var doesn't have an annotation an why it isn't in referenced vars?
>
>
> Ciao,
> Michael.