Re: [RFC] ipa bitwise constant propagation

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

On 12 August 2016 at 19:33, Jan Hubicka <hubicka@ucw.cz> wrote:
>> 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.
Thanks, I updated the patch to address these issues (attached).
However the patch caused ICE during testing
objc.dg/torture/forward-1.m (and few others but with same ICE):

Command line options:
/home/prathamesh.kulkarni/gnu-toolchain/gcc/bits-prop-5/bootstrap-build/gcc/xgcc
-B/home/prathamesh.kulkarni/gnu-toolchain/gcc/bits-prop-5/bootstrap-build/gcc/
/home/prathamesh.kulkarni/gnu-toolchain/gcc/bits-prop-5/gcc/gcc/testsuite/objc.dg/torture/forward-1.m
-fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -flto
-fuse-linker-plugin -fno-fat-lto-objects -fgnu-runtime
-I/home/prathamesh.kulkarni/gnu-toolchain/gcc/bits-prop-5/gcc/gcc/testsuite/../../libobjc
-B/home/prathamesh.kulkarni/gnu-toolchain/gcc/bits-prop-5/bootstrap-build/x86_64-pc-linux-gnu/./libobjc/.libs
-L/home/prathamesh.kulkarni/gnu-toolchain/gcc/bits-prop-5/bootstrap-build/x86_64-pc-linux-gnu/./libobjc/.libs
-lobjc -lm -o ./forward-1.exe

Backtrace:
0x8c0ed2 ipa_get_param_decl_index_1
../../gcc/gcc/ipa-prop.c:106
0x8b7dbb will_be_nonconstant_predicate
../../gcc/gcc/ipa-inline-analysis.c:2110
0x8b7dbb estimate_function_body_sizes
../../gcc/gcc/ipa-inline-analysis.c:2739
0x8bae26 compute_inline_parameters(cgraph_node*, bool)
../../gcc/gcc/ipa-inline-analysis.c:3030
0x8bb309 inline_analyze_function(cgraph_node*)
../../gcc/gcc/ipa-inline-analysis.c:4157
0x11dc402 ipa_icf::sem_function::merge(ipa_icf::sem_item*)
../../gcc/gcc/ipa-icf.c:1345
0x11d6334 ipa_icf::sem_item_optimizer::merge_classes(unsigned int)
../../gcc/gcc/ipa-icf.c:3461
0x11e12c6 ipa_icf::sem_item_optimizer::execute()
../../gcc/gcc/ipa-icf.c:2636
0x11e34d6 ipa_icf_driver
../../gcc/gcc/ipa-icf.c:3538
0x11e34d6 ipa_icf::pass_ipa_icf::execute(function*)
../../gcc/gcc/ipa-icf.c:3585

This appears due to following assert in ipa_get_param_decl_index_1():
gcc_checking_assert (!flag_wpa);
which was added by Martin's patch introducing ipa_get_type().
Removing the assert works, however I am not sure if that's the correct thing.
I would be grateful for suggestions on how to handle this case.

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

Attachment: bits-prop-5.diff
Description: Text document