Re: [RFC] ipa bitwise constant propagation

[Jan Hubicka <hubicka@ucw.cz>]

> On 11 August 2016 at 18:25, Jan Hubicka <hubicka@ucw.cz> wrote:
> >> @@ -266,6 +267,38 @@ private:
> >>    bool meet_with_1 (unsigned new_align, unsigned new_misalign);
> >>  };
> >>
> >> +/* Lattice of known bits, only capable of holding one value.
> >> +   Similar to ccp_lattice_t, mask represents which bits of value are constant.
> >> +   If a bit in mask is set to 0, then the corresponding bit in
> >> +   value is known to be constant.  */
> >> +
> >> +class ipcp_bits_lattice
> >> +{
> >> +public:
> >> +  bool bottom_p () { return m_lattice_val == IPA_BITS_VARYING; }
> >> +  bool top_p () { return m_lattice_val == IPA_BITS_UNDEFINED; }
> >> +  bool constant_p () { return m_lattice_val == IPA_BITS_CONSTANT; }
> >> +  bool set_to_bottom ();
> >> +  bool set_to_constant (widest_int, widest_int);
> >> +
> >> +  widest_int get_value () { return m_value; }
> >> +  widest_int get_mask () { return m_mask; }
> >> +
> >> +  bool meet_with (ipcp_bits_lattice& other, unsigned, signop,
> >> +               enum tree_code, tree);
> >> +
> >> +  bool meet_with (widest_int, widest_int, unsigned);
> >> +
> >> +  void print (FILE *);
> >> +
> >> +private:
> >> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } m_lattice_val;
> >> +  widest_int m_value, m_mask;
> >
> > Please add comment for these, like one in tree-ssa-ccp and mention they are the same
> > values.
> >
> >> +
> >> +  /* For K&R C programs, ipa_get_type() could return NULL_TREE.
> >> +     Avoid the transform for these cases.  */
> >> +  if (!parm_type)
> >> +    return dest_lattice->set_to_bottom ();
> >
> > Please add TDF_DETAILS dump for this so we notice if we drop useful info for no
> > good reasons.  It also happens for variadic functions but hopefully not much more.
> >
> > The patch is OK with those changes.
> Hi,
> The patch broke bootstrap due to segfault while compiling libsupc++/eh_alloc.cc
> in ipa_get_type() because callee_info->descriptors had 0 length in
> propagate_bits_accross_call.
> 
> After debugging a bit, I realized it was incorrect to use cs->callee and
> using cs->callee->function_symbol() fixed it:
> (that seemed to match with value of 'callee' variable in
> propagate_constants_accross_call).

Yes, callee may be alias and in that case you want to look into its target.
> 
> -  struct ipa_node_params *callee_info = IPA_NODE_REF (cs->callee);
> +  enum availability availability;
> +  cgraph_node *callee = cs->callee->function_symbol (&availability);
> +  struct ipa_node_params *callee_info = IPA_NODE_REF (callee);
>    tree parm_type = ipa_get_type (callee_info, idx);
> 
> Similarly I wonder if cs->caller->function_symbol() should be used
> instead of cs->caller in following place while obtaining lattices of
> source param ?
> 
>   if (jfunc->type == IPA_JF_PASS_THROUGH)
>     {
>       struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);

For callers you do not need to do that, because only real functions can call
(not aliases).
> 
> 
> The patch segfaults with -flto for gcc.c-torture/execute/920302-1.c in
> ipcp_propagate_stage ()
> while populating info->descriptors[k].decl_or_type because t becomes
> NULL and we dereference
> it with TREE_VALUE (t)
> The test-case has K&R style param declaration.
> The following change seems to fix it for me:
> 
> @@ -3235,7 +3235,7 @@ ipcp_propagate_stage (struct ipa_topo_info *topo)
>      if (in_lto_p)
>        {
>         tree t = TYPE_ARG_TYPES (TREE_TYPE (node->decl));
> -       for (int k = 0; k < ipa_get_param_count (info); k++)
> +       for (int k = 0; k < ipa_get_param_count (info) && t; k++)
>           {
>             gcc_assert (t != void_list_node);
>             info->descriptors[k].decl_or_type = TREE_VALUE (t);
> 
> Is that change OK ?

Yes, this also looks fine to me.

Honza
> 
> PS: I am on vacation for next week, will get back to working on the
> patch after returning.
> 
> Thanks,
> Prathamesh
> >
> > thanks,
> > Honza