Re: [RFC] ipa bitwise constant propagation

[Christophe Lyon]

On 26 August 2016 at 19:22, Prathamesh Kulkarni
 wrote:
> On 26 August 2016 at 21:53, Rainer Orth  wrote:
>> Hi Prathamesh,
>>
>>> The attached version passes bootstrap+test on
>>> x86_64-unknown-linux-gnu, ppc64le-linux-gnu,
>>> and with c,c++,fortran on armv8l-linux-gnueabihf.
>>> Cross-tested on arm*-*-* and aarch64*-*-*.
>>> Verified the patch survives lto-bootstrap on x86_64-unknown-linux-gnu.
>>> Ok to commit ?
>> [...]
>>> testsuite/
>>>   * gcc.dg/ipa/propbits-1.c: New test-case.
>>>   * gcc.dg/ipa/propbits-2.c: Likewise.
>>>   * gcc.dg/ipa/propbits-3.c: Likewise.
>> [...]
>>> diff --git a/gcc/testsuite/gcc.dg/ipa/propbits-2.c 
>>> b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
>>> new file mode 100644
>>> index 000..3a960f0
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
>>> @@ -0,0 +1,41 @@
>>> +/* x's mask should be meet(0xc, 0x3) == 0xf  */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp" } */
>>> +
>>> +extern int pass_test ();
>>> +extern int fail_test ();
>>> +
>>> +__attribute__((noinline))
>>> +static int f1(int x)
>>> +{
>>> +  if ((x & ~0xf) == 0)
>>> +return pass_test ();
>>> +  else
>>> +return fail_test ();
>>> +}
>>> +
>>> +__attribute__((noinline))
>>> +static int f2(int y)
>>> +{
>>> +  return f1(y & 0x03);
>>> +}
>>> +
>>> +__attribute__((noinline))
>>> +static int f3(int z)
>>> +{
>>> +  return f1(z & 0xc);
>>> +}
>>> +
>>> +extern int a;
>>> +extern int b;
>>> +
>>> +int main(void)
>>> +{
>>> +  int k = f2(a);
>>> +  int l = f3(b);
>>> +  return k + l;
>>> +}
>>> +
>>> +/* { dg-final { scan-ipa-dump "Adjusting mask for param 0 to 0xf" "cp" } } 
>>> */
>>> +/* { dg-final { scan-dump-tree-not "fail_test" "optimized" } } */
>>
>> This testcase thoroughly broke make check-gcc:
> Oops, sorry for the breakage. I am not sure how this missed my testing :/
> I obtained test results using test_summary script with and without patch,
> and compared the results with compare_tests which apparently showed no
> regressions...
> Thanks for the fix.
>

Hmmm that's weird indeed.

> Thanks,
> Prathamesh
>>
>> At first, runtest errors out with
>>
>> ERROR: (DejaGnu) proc "scan-dump-tree-not fail_test optimized" does not 
>> exist.

I do see this message in gcc.log (and in gcc.sum), but...
>>
>> The resulting incomplete gcc.sum files confuse dg-extract-results.py
>>
>> testsuite/gcc6/gcc.sum.sep: no recognised summary line
>> testsuite/gcc6/gcc.log.sep: no recognised summary line
>>
 I do not see this...

>> and cause it to emit en empty gcc.sum, effectively losing all gcc
>> testresults in mail-report.log.
and gcc.sum looks quite good (except for the ERROR: message
which is not noticed by the comparison tools).

It could be an effect of a different 'make -j' value, resulting
in different split of gcc.sum.sep, thus making the error
un-noticed.

Christophe

>> This cannot have been tested in any reasonable way.
>>
>> Once you fix the typo (scan-dump-tree-not -> scan-tree-dump-not), at
>> least we get a complete gcc.sum again, but the testcase still shows up as
>>
>> UNRESOLVED: gcc.dg/ipa/propbits-2.c scan-tree-dump-not optimized "fail_test"
>>
>> and gcc.log shows
>>
>> gcc.dg/ipa/propbits-2.c: dump file does not exist
>>
>> Adding -fdump-tree-optimized creates the necessary dump and finally lets
>> the test pass.
>>
>> Here's the resulting patch.  Unless there are objections, I plan to
>> commit it soon.
>>
>> Rainer
>>
>>
>> 2016-08-26  Rainer Orth  
>>
>> * gcc.dg/ipa/propbits-2.c: Add -fdump-tree-optimized to dg-options.
>> Fix typo.
>>
>>
>>
>> --
>> -
>> Rainer Orth, Center for Biotechnology, Bielefeld University
>>

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 26 August 2016 at 21:53, Rainer Orth  wrote:
> Hi Prathamesh,
>
>> The attached version passes bootstrap+test on
>> x86_64-unknown-linux-gnu, ppc64le-linux-gnu,
>> and with c,c++,fortran on armv8l-linux-gnueabihf.
>> Cross-tested on arm*-*-* and aarch64*-*-*.
>> Verified the patch survives lto-bootstrap on x86_64-unknown-linux-gnu.
>> Ok to commit ?
> [...]
>> testsuite/
>>   * gcc.dg/ipa/propbits-1.c: New test-case.
>>   * gcc.dg/ipa/propbits-2.c: Likewise.
>>   * gcc.dg/ipa/propbits-3.c: Likewise.
> [...]
>> diff --git a/gcc/testsuite/gcc.dg/ipa/propbits-2.c 
>> b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
>> new file mode 100644
>> index 000..3a960f0
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
>> @@ -0,0 +1,41 @@
>> +/* x's mask should be meet(0xc, 0x3) == 0xf  */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp" } */
>> +
>> +extern int pass_test ();
>> +extern int fail_test ();
>> +
>> +__attribute__((noinline))
>> +static int f1(int x)
>> +{
>> +  if ((x & ~0xf) == 0)
>> +return pass_test ();
>> +  else
>> +return fail_test ();
>> +}
>> +
>> +__attribute__((noinline))
>> +static int f2(int y)
>> +{
>> +  return f1(y & 0x03);
>> +}
>> +
>> +__attribute__((noinline))
>> +static int f3(int z)
>> +{
>> +  return f1(z & 0xc);
>> +}
>> +
>> +extern int a;
>> +extern int b;
>> +
>> +int main(void)
>> +{
>> +  int k = f2(a);
>> +  int l = f3(b);
>> +  return k + l;
>> +}
>> +
>> +/* { dg-final { scan-ipa-dump "Adjusting mask for param 0 to 0xf" "cp" } } 
>> */
>> +/* { dg-final { scan-dump-tree-not "fail_test" "optimized" } } */
>
> This testcase thoroughly broke make check-gcc:
Oops, sorry for the breakage. I am not sure how this missed my testing :/
I obtained test results using test_summary script with and without patch,
and compared the results with compare_tests which apparently showed no
regressions...
Thanks for the fix.

Thanks,
Prathamesh
>
> At first, runtest errors out with
>
> ERROR: (DejaGnu) proc "scan-dump-tree-not fail_test optimized" does not exist.
>
> The resulting incomplete gcc.sum files confuse dg-extract-results.py
>
> testsuite/gcc6/gcc.sum.sep: no recognised summary line
> testsuite/gcc6/gcc.log.sep: no recognised summary line
>
> and cause it to emit en empty gcc.sum, effectively losing all gcc
> testresults in mail-report.log.
>
> This cannot have been tested in any reasonable way.
>
> Once you fix the typo (scan-dump-tree-not -> scan-tree-dump-not), at
> least we get a complete gcc.sum again, but the testcase still shows up as
>
> UNRESOLVED: gcc.dg/ipa/propbits-2.c scan-tree-dump-not optimized "fail_test"
>
> and gcc.log shows
>
> gcc.dg/ipa/propbits-2.c: dump file does not exist
>
> Adding -fdump-tree-optimized creates the necessary dump and finally lets
> the test pass.
>
> Here's the resulting patch.  Unless there are objections, I plan to
> commit it soon.
>
> Rainer
>
>
> 2016-08-26  Rainer Orth  
>
> * gcc.dg/ipa/propbits-2.c: Add -fdump-tree-optimized to dg-options.
> Fix typo.
>
>
>
> --
> -
> Rainer Orth, Center for Biotechnology, Bielefeld University
>

Re: [RFC] ipa bitwise constant propagation

[Rainer Orth]

Hi Prathamesh,

> The attached version passes bootstrap+test on
> x86_64-unknown-linux-gnu, ppc64le-linux-gnu,
> and with c,c++,fortran on armv8l-linux-gnueabihf.
> Cross-tested on arm*-*-* and aarch64*-*-*.
> Verified the patch survives lto-bootstrap on x86_64-unknown-linux-gnu.
> Ok to commit ?
[...]
> testsuite/
>   * gcc.dg/ipa/propbits-1.c: New test-case.
>   * gcc.dg/ipa/propbits-2.c: Likewise.
>   * gcc.dg/ipa/propbits-3.c: Likewise.
[...]
> diff --git a/gcc/testsuite/gcc.dg/ipa/propbits-2.c 
> b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
> new file mode 100644
> index 000..3a960f0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
> @@ -0,0 +1,41 @@
> +/* x's mask should be meet(0xc, 0x3) == 0xf  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp" } */
> +
> +extern int pass_test ();
> +extern int fail_test ();
> +
> +__attribute__((noinline))
> +static int f1(int x)
> +{
> +  if ((x & ~0xf) == 0)
> +return pass_test ();
> +  else
> +return fail_test ();
> +}
> +
> +__attribute__((noinline))
> +static int f2(int y)
> +{
> +  return f1(y & 0x03);
> +}
> +
> +__attribute__((noinline))
> +static int f3(int z)
> +{
> +  return f1(z & 0xc);
> +}
> +
> +extern int a;
> +extern int b;
> +
> +int main(void)
> +{
> +  int k = f2(a); 
> +  int l = f3(b);
> +  return k + l;
> +}
> +
> +/* { dg-final { scan-ipa-dump "Adjusting mask for param 0 to 0xf" "cp" } } */
> +/* { dg-final { scan-dump-tree-not "fail_test" "optimized" } } */

This testcase thoroughly broke make check-gcc:

At first, runtest errors out with

ERROR: (DejaGnu) proc "scan-dump-tree-not fail_test optimized" does not exist.

The resulting incomplete gcc.sum files confuse dg-extract-results.py

testsuite/gcc6/gcc.sum.sep: no recognised summary line
testsuite/gcc6/gcc.log.sep: no recognised summary line

and cause it to emit en empty gcc.sum, effectively losing all gcc
testresults in mail-report.log.

This cannot have been tested in any reasonable way.

Once you fix the typo (scan-dump-tree-not -> scan-tree-dump-not), at
least we get a complete gcc.sum again, but the testcase still shows up as

UNRESOLVED: gcc.dg/ipa/propbits-2.c scan-tree-dump-not optimized "fail_test"

and gcc.log shows

gcc.dg/ipa/propbits-2.c: dump file does not exist

Adding -fdump-tree-optimized creates the necessary dump and finally lets
the test pass.

Here's the resulting patch.  Unless there are objections, I plan to
commit it soon.

Rainer


2016-08-26  Rainer Orth  

* gcc.dg/ipa/propbits-2.c: Add -fdump-tree-optimized to dg-options.
Fix typo.

diff --git a/gcc/testsuite/gcc.dg/ipa/propbits-2.c b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
--- a/gcc/testsuite/gcc.dg/ipa/propbits-2.c
+++ b/gcc/testsuite/gcc.dg/ipa/propbits-2.c
@@ -1,7 +1,7 @@
 /* x's mask should be meet(0xc, 0x3) == 0xf  */
 
 /* { dg-do compile } */
-/* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp" } */
+/* { dg-options "-O2 -fno-early-inlining -fdump-ipa-cp -fdump-tree-optimized" } */
 
 extern int pass_test ();
 extern int fail_test ();
@@ -38,4 +38,4 @@ int main(void)
 }
 
 /* { dg-final { scan-ipa-dump "Adjusting mask for param 0 to 0xf" "cp" } } */
-/* { dg-final { scan-dump-tree-not "fail_test" "optimized" } } */
+/* { dg-final { scan-tree-dump-not "fail_test" "optimized" } } */

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 25 August 2016 at 19:14, Jan Hubicka  wrote:
>> Patch for performing interprocedural bitwise constant propagation.
>>
>> 2016-08-23  Prathamesh Kulkarni  
>>   Martin Jambhor  
>>
>>   * common.opt: New option -fipa-cp-bit.
>>   * doc/invoke.texi: Document -fipa-cp-bit.
>>   * opts.c (default_options_table): Add entry for -fipa-cp-bit.
>
> Bitwise intraprocedural ccp is enabled by -ftree-bit-cp, so I think the
> option name should be -fipa-bit-cp so things are more consistent.
>
> Patch is OK with this change.
Thanks, committed the patch as r239769.
As next steps, I will try to merge bitwise and pointer-alignment propagation.

Thanks,
Prathamesh
>
> Thanks!
> Honza
>>   (enable_fdo_optimizations): Check for flag_ipa_cp_bit.
>>   * tree-ssa-ccp.h: New header file.
>>   * tree-ssa-ccp.c: Include tree-ssa-ccp.h
>>   (bit_value_binop_1): Change to bit_value_binop_1 and export it.
>>   Replace all occurences of tree parameter by two new params: signop, 
>> int.
>>   (bit_value_unop_1): Change to bit_value_unop and export it.
>>   Replace all occurences of tree parameter by two new params: signop,
>>   int.
>>   (bit_value_binop): Change call from bit_value_binop_1 to
>>   bit_value_binop.
>>   (bit_value_assume_aligned): Likewise.
>>   (bit_value_unop): Change call from bit_value_unop_1 to bit_value_unop.
>>   (do_ssa_ccp): Pass nonzero_p || flag_ipa_cp_bit instead of nonzero_p
>>   to ccp_finalize.
>>   (ccp_finalize): Skip processing if val->mask == 0.
>>   * ipa-cp.c: Include tree-ssa-ccp.h
>>   (ipcp_bits_lattice): New class.
>>   (ipcp_param_lattice (bits_lattice): New member.
>>   (print_all_lattices): Call ipcp_bits_lattice::print.
>>   (set_all_contains_variable): Call ipcp_bits_lattice::set_to_bottom.
>>   (initialize_node_lattices): Likewise.
>>   (propagate_bits_accross_jump_function): New function.
>>   (propagate_constants_accross_call): Call
>>   propagate_bits_accross_jump_function.
>>   (ipcp_propagate_stage): Store parameter types when in_lto_p is true.
>>   (ipcp_store_bits_results): New function.
>>   (ipcp_driver): Call ipcp_store_bits_results.
>>   * ipa-prop.h (ipa_bits): New struct.
>>   (ipa_jump_func): Add new member bits of type ipa_bits.
>>   (ipa_param_descriptor): Change decl to decl_or_type.
>>   (ipa_get_param): Change decl to decl_or_type and assert on
>>   PARM_DECL.
>>   (ipa_get_type): New function.
>>   (ipcp_transformation_summary): New member bits.
>>   * ipa-prop.c (ipa_get_param_decl_index_1): s/decl/decl_or_type.
>>   (ipa_populate_param_decls): Likewise.
>>   (ipa_dump_param): Likewise.
>>   (ipa_print_node_jump_functions_for_edge): Pretty-print ipa_bits jump
>>   function.
>>   (ipa_set_jf_unknown): Set ipa_bits::known to false.
>>   (ipa_compute_jump_functions_for_edge): Compute jump function for bits
>>   propagation.
>>   (ipa_node_params_t::duplicate): Copy src->bits into dst->bits.
>>   (ipa_write_jump_function): Add streaming for ipa_bits.
>>   (ipa_read_jump_function): Add support for reading streamed ipa_bits.
>>   (write_ipcp_transformation_info): Add streaming for ipa_bits
>>   summary for ltrans.
>>   (read_ipcp_transfomration_info): Add support for reading streamed 
>> ipa_bits.
>>   (ipcp_update_bits): New function.
>>   (ipcp_transform_function): Call ipcp_update_bits.
>>
>

Re: [RFC] ipa bitwise constant propagation

[Jan Hubicka]

> Patch for performing interprocedural bitwise constant propagation.
> 
> 2016-08-23  Prathamesh Kulkarni  
>   Martin Jambhor  
> 
>   * common.opt: New option -fipa-cp-bit.
>   * doc/invoke.texi: Document -fipa-cp-bit.
>   * opts.c (default_options_table): Add entry for -fipa-cp-bit.

Bitwise intraprocedural ccp is enabled by -ftree-bit-cp, so I think the
option name should be -fipa-bit-cp so things are more consistent.

Patch is OK with this change.

Thanks!
Honza
>   (enable_fdo_optimizations): Check for flag_ipa_cp_bit.
>   * tree-ssa-ccp.h: New header file.
>   * tree-ssa-ccp.c: Include tree-ssa-ccp.h
>   (bit_value_binop_1): Change to bit_value_binop_1 and export it.
>   Replace all occurences of tree parameter by two new params: signop, int.
>   (bit_value_unop_1): Change to bit_value_unop and export it.
>   Replace all occurences of tree parameter by two new params: signop,
>   int.
>   (bit_value_binop): Change call from bit_value_binop_1 to
>   bit_value_binop.
>   (bit_value_assume_aligned): Likewise.
>   (bit_value_unop): Change call from bit_value_unop_1 to bit_value_unop.
>   (do_ssa_ccp): Pass nonzero_p || flag_ipa_cp_bit instead of nonzero_p
>   to ccp_finalize.
>   (ccp_finalize): Skip processing if val->mask == 0.
>   * ipa-cp.c: Include tree-ssa-ccp.h
>   (ipcp_bits_lattice): New class.
>   (ipcp_param_lattice (bits_lattice): New member.
>   (print_all_lattices): Call ipcp_bits_lattice::print.
>   (set_all_contains_variable): Call ipcp_bits_lattice::set_to_bottom. 
>   (initialize_node_lattices): Likewise.
>   (propagate_bits_accross_jump_function): New function.
>   (propagate_constants_accross_call): Call
>   propagate_bits_accross_jump_function.
>   (ipcp_propagate_stage): Store parameter types when in_lto_p is true.
>   (ipcp_store_bits_results): New function.
>   (ipcp_driver): Call ipcp_store_bits_results.
>   * ipa-prop.h (ipa_bits): New struct.
>   (ipa_jump_func): Add new member bits of type ipa_bits.
>   (ipa_param_descriptor): Change decl to decl_or_type.
>   (ipa_get_param): Change decl to decl_or_type and assert on
>   PARM_DECL.
>   (ipa_get_type): New function.
>   (ipcp_transformation_summary): New member bits.
>   * ipa-prop.c (ipa_get_param_decl_index_1): s/decl/decl_or_type.
>   (ipa_populate_param_decls): Likewise.
>   (ipa_dump_param): Likewise.
>   (ipa_print_node_jump_functions_for_edge): Pretty-print ipa_bits jump
>   function.
>   (ipa_set_jf_unknown): Set ipa_bits::known to false.
>   (ipa_compute_jump_functions_for_edge): Compute jump function for bits
>   propagation.
>   (ipa_node_params_t::duplicate): Copy src->bits into dst->bits.
>   (ipa_write_jump_function): Add streaming for ipa_bits.
>   (ipa_read_jump_function): Add support for reading streamed ipa_bits.
>   (write_ipcp_transformation_info): Add streaming for ipa_bits
>   summary for ltrans.
>   (read_ipcp_transfomration_info): Add support for reading streamed 
> ipa_bits.
>   (ipcp_update_bits): New function.
>   (ipcp_transform_function): Call ipcp_update_bits.
> 

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 22 August 2016 at 19:24, Prathamesh Kulkarni
 wrote:
> On 22 August 2016 at 19:03, Martin Jambor  wrote:
>> Hi,
>>
>> On Tue, Aug 16, 2016 at 06:34:48PM +0530, Prathamesh Kulkarni wrote:
>>> 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.
>>>
>>
>> I wrote that the patch was not really tested, I did not think about
>> ICF loading bodies and re-running body-analyses at WPO time.
>> Nevertheless, after some consideration, I think that just removing the
>> assert is fine.  After all, the caller must have passed a PARM_DECL if
>> it is doing anything sensible at all and that means we have access to
>> the function body.
> Thanks for the pointers. I will validate the patch after removing assert,
> and get back.
Hi,
The attached version passes bootstrap+test on
x86_64-unknown-linux-gnu, ppc64le-linux-gnu,
and with c,c++,fortran on armv8l-linux-gnueabihf.
Cross-tested on arm*-*-* and aarch64*-*-*.
Verified the patch survives lto-bootstrap on x86_64-unknown-linux-gnu.
Ok to commit ?

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
>>
>> Thanks,
>>
>> Martin
Patch for performing interprocedural bitwise constant propagation.

2016-08-23  Prathamesh Kulkarni  
Martin Jambhor  

* common.opt: New option -fipa-cp-bit.
* doc/invoke.texi: Document -fipa-cp-bit.
* opts.c (default_options_table): Add entry for -fipa-cp-bit.
(enable_fdo_optimizations): Check for flag_ipa_cp_bit.
* tree-ssa-ccp.h: New header file.
* tree-ssa-ccp.c: Include tree-ssa-ccp.h
(bit_value_binop_1): Change to bit_value_binop_1 and export it.
Replace all occurences of tree parameter by two new params: signop, int.
(bit_value_unop_1): Change to bit_value_unop and export it.
Replace all occurences of tree parameter by two new params: signop,
int.
(bit_value_binop): Change call from bit_value_binop_1 to
bit_value_binop.
(bit_value_assume_aligned): Likewise.
(bit_value_unop): Change call from bit_value_unop_1 to bit_value_unop.
(do_ssa_ccp): Pass nonzero_p || flag_ipa_cp_bit instead of nonzero_p
to ccp_finalize.
(ccp_finalize): Skip processing if val->mask == 0.
* ipa-cp.c: Include tree-ssa-ccp.h
(ipcp_bits_lattice): New class.
(ipcp_param_lattice (bits_lattice): New member.
(print_all_lattices): Call ipcp_bits_lattice::print.
(set_all_contains_variable): Call ipcp_bits_lattice::set_to_bottom. 
(initialize_node_lattices): Likewise.
(propagate_bits_accross_jump_function): New function.
(propagate_constants_accross_call): Call
propagate_bits_accross_jump_function.
(ipcp_propagate_stage): Store parameter types 

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 22 August 2016 at 19:03, Martin Jambor  wrote:
> Hi,
>
> On Tue, Aug 16, 2016 at 06:34:48PM +0530, Prathamesh Kulkarni wrote:
>> 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.
>>
>
> I wrote that the patch was not really tested, I did not think about
> ICF loading bodies and re-running body-analyses at WPO time.
> Nevertheless, after some consideration, I think that just removing the
> assert is fine.  After all, the caller must have passed a PARM_DECL if
> it is doing anything sensible at all and that means we have access to
> the function body.
Thanks for the pointers. I will validate the patch after removing assert,
and get back.

Thanks,
Prathamesh
>
> Thanks,
>
> Martin

Re: [RFC] ipa bitwise constant propagation

[Martin Jambor]

Hi,

On Tue, Aug 16, 2016 at 06:34:48PM +0530, Prathamesh Kulkarni wrote:
> 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.
> 

I wrote that the patch was not really tested, I did not think about
ICF loading bodies and re-running body-analyses at WPO time.
Nevertheless, after some consideration, I think that just removing the
assert is fine.  After all, the caller must have passed a PARM_DECL if
it is doing anything sensible at all and that means we have access to
the function body.

Thanks,

Martin

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 12 August 2016 at 19:33, Jan Hubicka  wrote:
>> On 11 August 2016 at 18:25, Jan Hubicka  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 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 ();
>> +  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 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 

Re: [RFC] ipa bitwise constant propagation

[Jan Hubicka]

> On 11 August 2016 at 18:25, Jan Hubicka  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 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 ();
> +  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 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

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 11 August 2016 at 18:25, Jan Hubicka  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 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).

-  struct ipa_node_params *callee_info = IPA_NODE_REF (cs->callee);
+  enum availability availability;
+  cgraph_node *callee = cs->callee->function_symbol ();
+  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);


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 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 ?

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

Thanks,
Prathamesh
>
> thanks,
> Honza

Re: [RFC] ipa bitwise constant propagation

[Jan Hubicka]

> @@ -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 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.

thanks,
Honza

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 10 August 2016 at 14:14, Prathamesh Kulkarni
 wrote:
> On 9 August 2016 at 23:43, Martin Jambor  wrote:
>> Hi,
>>
>> On Tue, Aug 09, 2016 at 05:17:31PM +0530, Prathamesh Kulkarni wrote:
>>> On 9 August 2016 at 16:39, Martin Jambor  wrote:
>>>
>>> ...
>>>
>>> >> Instead of storing arg's precision and sign, we should store
>>> >> parameter's precision and sign in ipa_compute_jump_functions_for_edge ().
>>> >> Diff with respect to previous patch:
>>> >>
>>> >> @@ -1688,9 +1690,9 @@ ipa_compute_jump_functions_for_edge (struct
>>> >> ipa_func_body_info *fbi,
>>> >>&& (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
>>> >>   {
>>> >>jfunc->bits.known = true;
>>> >> -  jfunc->bits.sgn = TYPE_SIGN (TREE_TYPE (arg));
>>> >> -  jfunc->bits.precision = TYPE_PRECISION (TREE_TYPE (arg));
>>> >> -
>>> >> +  jfunc->bits.sgn = TYPE_SIGN (param_type);
>>> >> +  jfunc->bits.precision = TYPE_PRECISION (param_type);
>>> >> +
>>> >
>>> > If you want to use the precision of the formal parameter then you do
>>> > not need to store it to jump functions.  Parameter DECLs along with
>>> > their types are readily accessible in IPA (even with LTO).  It would
>>> > also be much clearer what is going on, IMHO.
>>> Could you please point out how to access parameter decl in wpa ?
>>> The only reason I ended up putting this in jump function was because
>>> I couldn't figure out how to access param decl during WPA.
>>> I see there's ipa_get_param() in ipa-prop.h however it's gated on
>>> gcc_checking_assert (!flag_wpa), so I suppose I can't use this
>>> during WPA ?
>>>
>>> Alternatively I think I could access cs->callee->decl and get to the param 
>>> decl
>>> by walking DECL_ARGUMENTS ?
>>
>> Actually, we no longer have DECL_ARGUMENTS during LTO WPA.  But in
>> most cases, you can still get at the type with something like the
>> following (only very lightly tested) patch, if Honza does not think it
>> is too crazy.
>>
>> Note that= for old K C sources we do not have TYPE_ARG_TYPES and so
>> ipa_get_type can return NULL(!) ...however I wonder whether for such
>> programs the type assumptions made in callers when constructing jump
>> functions can be trusted either.
>>
>> I have to run, we will continue the discussion later.
> Thanks for the patch.
> In this version, I updated the patch to use ipa_get_type, remove
> precision and sgn
> from ipcp_bits_lattice and ipa_bits, and renamed member variables to
> add m_ prefix.
> Does it look OK ?
> I am looking for test-case that affects precision and hopefully add
> that along with other
> test-cases in follow-up patch.
> Bootstrap+test in progress on x86_64-unknown-linux-gnu.
oops, I forgot to add tree-ssa-ccp.h hunk to the patch :/
Attached in this version.

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
>>
>> Martin
>>
>>
>> 2016-08-09  Martin Jambor  
>>
>> * ipa-prop.h (ipa_param_descriptor): Renamed decl to decl_or_type.
>> Update comment.
>> (ipa_get_param): Updated comment, added assert that we have a
>> PARM_DECL.
>> (ipa_get_type): New function.
>> * ipa-cp.c (ipcp_propagate_stage): Fill in argument types in LTO 
>> mode.
>> * ipa-prop.c (ipa_get_param_decl_index_1): Use decl_or_type
>> instead of decl;
>> (ipa_populate_param_decls): Likewise.
>> (ipa_dump_param): Likewise.
>>
>>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index 5b6cb9a..3465da5 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -1952,11 +1952,21 @@ propagate_constants_accross_call (struct cgraph_edge 
>> *cs)
>>else
>>  i = 0;
>>
>> +  /* !!! The following dump is of course only a demonstration that it 
>> works: */
>> +  debug_generic_expr (callee->decl);
>> +  fprintf (stderr, "\n");
>> +
>>for (; (i < args_count) && (i < parms_count); i++)
>>  {
>>struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>>struct ipcp_param_lattices *dest_plats;
>>
>> +  /* !!! The following dump is of course only a demonstration that it
>> + works: */
>> +  fprintf (stderr, "  The type of parameter %i is: ", i);
>> +  debug_generic_expr (ipa_get_type (callee_info, i));
>> +  fprintf (stderr, "\n");
>> +
>>dest_plats = ipa_get_parm_lattices (callee_info, i);
>>if (availability == AVAIL_INTERPOSABLE)
>> ret |= set_all_contains_variable (dest_plats);
>> @@ -2936,6 +2946,19 @@ ipcp_propagate_stage (struct ipa_topo_info *topo)
>>{
>>  struct ipa_node_params *info = IPA_NODE_REF (node);
>>
>> +/* In LTO we do not have PARM_DECLs but we would still like to be able 
>> to
>> +   look at types of parameters.  */
>> +if (in_lto_p)
>> +  {
>> +   tree t = TYPE_ARG_TYPES (TREE_TYPE (node->decl));
>> +   for (int k = 0; k < ipa_get_param_count (info); k++)
>> + {
>> +   gcc_assert (t != void_list_node);
>> + 

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 9 August 2016 at 23:43, Martin Jambor  wrote:
> Hi,
>
> On Tue, Aug 09, 2016 at 05:17:31PM +0530, Prathamesh Kulkarni wrote:
>> On 9 August 2016 at 16:39, Martin Jambor  wrote:
>>
>> ...
>>
>> >> Instead of storing arg's precision and sign, we should store
>> >> parameter's precision and sign in ipa_compute_jump_functions_for_edge ().
>> >> Diff with respect to previous patch:
>> >>
>> >> @@ -1688,9 +1690,9 @@ ipa_compute_jump_functions_for_edge (struct
>> >> ipa_func_body_info *fbi,
>> >>&& (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
>> >>   {
>> >>jfunc->bits.known = true;
>> >> -  jfunc->bits.sgn = TYPE_SIGN (TREE_TYPE (arg));
>> >> -  jfunc->bits.precision = TYPE_PRECISION (TREE_TYPE (arg));
>> >> -
>> >> +  jfunc->bits.sgn = TYPE_SIGN (param_type);
>> >> +  jfunc->bits.precision = TYPE_PRECISION (param_type);
>> >> +
>> >
>> > If you want to use the precision of the formal parameter then you do
>> > not need to store it to jump functions.  Parameter DECLs along with
>> > their types are readily accessible in IPA (even with LTO).  It would
>> > also be much clearer what is going on, IMHO.
>> Could you please point out how to access parameter decl in wpa ?
>> The only reason I ended up putting this in jump function was because
>> I couldn't figure out how to access param decl during WPA.
>> I see there's ipa_get_param() in ipa-prop.h however it's gated on
>> gcc_checking_assert (!flag_wpa), so I suppose I can't use this
>> during WPA ?
>>
>> Alternatively I think I could access cs->callee->decl and get to the param 
>> decl
>> by walking DECL_ARGUMENTS ?
>
> Actually, we no longer have DECL_ARGUMENTS during LTO WPA.  But in
> most cases, you can still get at the type with something like the
> following (only very lightly tested) patch, if Honza does not think it
> is too crazy.
>
> Note that= for old K C sources we do not have TYPE_ARG_TYPES and so
> ipa_get_type can return NULL(!) ...however I wonder whether for such
> programs the type assumptions made in callers when constructing jump
> functions can be trusted either.
>
> I have to run, we will continue the discussion later.
Thanks for the patch.
In this version, I updated the patch to use ipa_get_type, remove
precision and sgn
from ipcp_bits_lattice and ipa_bits, and renamed member variables to
add m_ prefix.
Does it look OK ?
I am looking for test-case that affects precision and hopefully add
that along with other
test-cases in follow-up patch.
Bootstrap+test in progress on x86_64-unknown-linux-gnu.

Thanks,
Prathamesh
>
> Martin
>
>
> 2016-08-09  Martin Jambor  
>
> * ipa-prop.h (ipa_param_descriptor): Renamed decl to decl_or_type.
> Update comment.
> (ipa_get_param): Updated comment, added assert that we have a
> PARM_DECL.
> (ipa_get_type): New function.
> * ipa-cp.c (ipcp_propagate_stage): Fill in argument types in LTO mode.
> * ipa-prop.c (ipa_get_param_decl_index_1): Use decl_or_type
> instead of decl;
> (ipa_populate_param_decls): Likewise.
> (ipa_dump_param): Likewise.
>
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 5b6cb9a..3465da5 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1952,11 +1952,21 @@ propagate_constants_accross_call (struct cgraph_edge 
> *cs)
>else
>  i = 0;
>
> +  /* !!! The following dump is of course only a demonstration that it works: 
> */
> +  debug_generic_expr (callee->decl);
> +  fprintf (stderr, "\n");
> +
>for (; (i < args_count) && (i < parms_count); i++)
>  {
>struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>struct ipcp_param_lattices *dest_plats;
>
> +  /* !!! The following dump is of course only a demonstration that it
> + works: */
> +  fprintf (stderr, "  The type of parameter %i is: ", i);
> +  debug_generic_expr (ipa_get_type (callee_info, i));
> +  fprintf (stderr, "\n");
> +
>dest_plats = ipa_get_parm_lattices (callee_info, i);
>if (availability == AVAIL_INTERPOSABLE)
> ret |= set_all_contains_variable (dest_plats);
> @@ -2936,6 +2946,19 @@ ipcp_propagate_stage (struct ipa_topo_info *topo)
>{
>  struct ipa_node_params *info = IPA_NODE_REF (node);
>
> +/* In LTO we do not have PARM_DECLs but we would still like to be able to
> +   look at types of parameters.  */
> +if (in_lto_p)
> +  {
> +   tree t = TYPE_ARG_TYPES (TREE_TYPE (node->decl));
> +   for (int k = 0; k < ipa_get_param_count (info); k++)
> + {
> +   gcc_assert (t != void_list_node);
> +   info->descriptors[k].decl_or_type = TREE_VALUE (t);
> +   t = t ? TREE_CHAIN (t) : NULL;
> + }
> +  }
> +
>  determine_versionability (node, info);
>  if (node->has_gimple_body_p ())
>{
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 132b622..1eaccdf 100644
> --- a/gcc/ipa-prop.c
> 

Re: [RFC] ipa bitwise constant propagation

[Martin Jambor]

Hi,

On Tue, Aug 09, 2016 at 05:17:31PM +0530, Prathamesh Kulkarni wrote:
> On 9 August 2016 at 16:39, Martin Jambor  wrote:
>
> ...
>
> >> Instead of storing arg's precision and sign, we should store
> >> parameter's precision and sign in ipa_compute_jump_functions_for_edge ().
> >> Diff with respect to previous patch:
> >>
> >> @@ -1688,9 +1690,9 @@ ipa_compute_jump_functions_for_edge (struct
> >> ipa_func_body_info *fbi,
> >>&& (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
> >>   {
> >>jfunc->bits.known = true;
> >> -  jfunc->bits.sgn = TYPE_SIGN (TREE_TYPE (arg));
> >> -  jfunc->bits.precision = TYPE_PRECISION (TREE_TYPE (arg));
> >> -
> >> +  jfunc->bits.sgn = TYPE_SIGN (param_type);
> >> +  jfunc->bits.precision = TYPE_PRECISION (param_type);
> >> +
> >
> > If you want to use the precision of the formal parameter then you do
> > not need to store it to jump functions.  Parameter DECLs along with
> > their types are readily accessible in IPA (even with LTO).  It would
> > also be much clearer what is going on, IMHO.
> Could you please point out how to access parameter decl in wpa ?
> The only reason I ended up putting this in jump function was because
> I couldn't figure out how to access param decl during WPA.
> I see there's ipa_get_param() in ipa-prop.h however it's gated on
> gcc_checking_assert (!flag_wpa), so I suppose I can't use this
> during WPA ?
> 
> Alternatively I think I could access cs->callee->decl and get to the param 
> decl
> by walking DECL_ARGUMENTS ?

Actually, we no longer have DECL_ARGUMENTS during LTO WPA.  But in
most cases, you can still get at the type with something like the
following (only very lightly tested) patch, if Honza does not think it
is too crazy.

Note that= for old K C sources we do not have TYPE_ARG_TYPES and so
ipa_get_type can return NULL(!) ...however I wonder whether for such
programs the type assumptions made in callers when constructing jump
functions can be trusted either.

I have to run, we will continue the discussion later.

Martin


2016-08-09  Martin Jambor  

* ipa-prop.h (ipa_param_descriptor): Renamed decl to decl_or_type.
Update comment.
(ipa_get_param): Updated comment, added assert that we have a
PARM_DECL.
(ipa_get_type): New function.
* ipa-cp.c (ipcp_propagate_stage): Fill in argument types in LTO mode.
* ipa-prop.c (ipa_get_param_decl_index_1): Use decl_or_type
instead of decl;
(ipa_populate_param_decls): Likewise.
(ipa_dump_param): Likewise.


diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 5b6cb9a..3465da5 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1952,11 +1952,21 @@ propagate_constants_accross_call (struct cgraph_edge 
*cs)
   else
 i = 0;
 
+  /* !!! The following dump is of course only a demonstration that it works: */
+  debug_generic_expr (callee->decl);
+  fprintf (stderr, "\n");
+
   for (; (i < args_count) && (i < parms_count); i++)
 {
   struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
   struct ipcp_param_lattices *dest_plats;
 
+  /* !!! The following dump is of course only a demonstration that it
+ works: */
+  fprintf (stderr, "  The type of parameter %i is: ", i);
+  debug_generic_expr (ipa_get_type (callee_info, i));
+  fprintf (stderr, "\n");
+
   dest_plats = ipa_get_parm_lattices (callee_info, i);
   if (availability == AVAIL_INTERPOSABLE)
ret |= set_all_contains_variable (dest_plats);
@@ -2936,6 +2946,19 @@ ipcp_propagate_stage (struct ipa_topo_info *topo)
   {
 struct ipa_node_params *info = IPA_NODE_REF (node);
 
+/* In LTO we do not have PARM_DECLs but we would still like to be able to
+   look at types of parameters.  */
+if (in_lto_p)
+  {
+   tree t = TYPE_ARG_TYPES (TREE_TYPE (node->decl));
+   for (int k = 0; k < ipa_get_param_count (info); k++)
+ {
+   gcc_assert (t != void_list_node);
+   info->descriptors[k].decl_or_type = TREE_VALUE (t);
+   t = t ? TREE_CHAIN (t) : NULL;
+ }
+  }
+
 determine_versionability (node, info);
 if (node->has_gimple_body_p ())
   {
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 132b622..1eaccdf 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -103,9 +103,10 @@ ipa_get_param_decl_index_1 (vec 
descriptors, tree ptree)
 {
   int i, count;
 
+  gcc_checking_assert (!flag_wpa);
   count = descriptors.length ();
   for (i = 0; i < count; i++)
-if (descriptors[i].decl == ptree)
+if (descriptors[i].decl_or_type == ptree)
   return i;
 
   return -1;
@@ -138,7 +139,7 @@ ipa_populate_param_decls (struct cgraph_node *node,
   param_num = 0;
   for (parm = fnargs; parm; parm = DECL_CHAIN (parm))
 {
-  descriptors[param_num].decl = parm;
+  descriptors[param_num].decl_or_type = parm;
   descriptors[param_num].move_cost = estimate_move_cost (TREE_TYPE 

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 9 August 2016 at 16:39, Martin Jambor  wrote:
> Hi,
>
> On Tue, Aug 09, 2016 at 01:41:21PM +0530, Prathamesh Kulkarni wrote:
>> On 8 August 2016 at 19:33, Martin Jambor  wrote:
>> >> >> +class ipcp_bits_lattice
>> >> >> +{
>> >> >> +public:
>> >> >> +  bool bottom_p () { return lattice_val == IPA_BITS_VARYING; }
>> >> >> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
>> >> >> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
>> >> >> +  bool set_to_bottom ();
>> >> >> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
>> >> >> +
>> >> >> +  widest_int get_value () { return value; }
>> >> >> +  widest_int get_mask () { return mask; }
>> >> >> +  signop get_sign () { return sgn; }
>> >> >> +  unsigned get_precision () { return precision; }
>> >> >> +
>> >> >> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
>> >> >> +  bool meet_with (widest_int, widest_int, signop, unsigned);
>> >> >> +
>> >> >> +  void print (FILE *);
>> >> >> +
>> >> >> +private:
>> >> >> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
>> >> >> lattice_val;
>> >> >> +  widest_int value, mask;
>> >> >> +  signop sgn;
>> >> >> +  unsigned precision;
>> >
>> > I know that the existing code in ipa-cp.c does not do this, but please
>> > prefix member variables with "m_" like our coding style guidelines
>> > suggest (or even require?).  You routinely reuse those same names in
>> > names of parameters of meet_with and I believe that is a practice that
>> > will sooner or later lead to confusing the two and bugs.
>> Sorry about this, will change to m_ prefix in followup patch.
>
> Thanks a lot.
>
>> >
>> >> >> +
>> >> >> +  bool meet_with_1 (widest_int, widest_int);
>> >> >> +  void get_value_and_mask (tree, widest_int *, widest_int *);
>> >> >> +};
>> >> >> +
>> >> >>  /* Structure containing lattices for a parameter itself and for 
>> >> >> pieces of
>> >> >> aggregates that are passed in the parameter or by a reference in a 
>> >> >> parameter
>> >> >> plus some other useful flags.  */
>> >> >> @@ -281,6 +316,8 @@ public:
>> >> >>ipcp_agg_lattice *aggs;
>> >> >>/* Lattice describing known alignment.  */
>> >> >>ipcp_alignment_lattice alignment;
>> >> >> +  /* Lattice describing known bits.  */
>> >> >> +  ipcp_bits_lattice bits_lattice;
>> >> >>/* Number of aggregate lattices */
>> >> >>int aggs_count;
>> >> >>/* True if aggregate data were passed by reference (as opposed to by
>> >> >> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
>> >> >>  fprintf (f, " Alignment %u, misalignment %u\n", align, 
>> >> >> misalign);
>> >> >>  }
>> >> >>
>> >>
>> >> ...
>> >>
>> >> >> +}
>> >> >> +
>> >> >> +/* Meet operation, similar to ccp_lattice_meet, we xor values
>> >> >> +   if this->value, value have different values at same bit positions, 
>> >> >> we want
>> >> >> +   to drop that bit to varying. Return true if mask is changed.
>> >> >> +   This function assumes that the lattice value is in CONSTANT state  
>> >> >> */
>> >> >> +
>> >> >> +bool
>> >> >> +ipcp_bits_lattice::meet_with_1 (widest_int value, widest_int mask)
>> >> >> +{
>> >> >> +  gcc_assert (constant_p ());
>> >> >> +
>> >> >> +  widest_int old_mask = this->mask;
>> >> >> +  this->mask = (this->mask | mask) | (this->value ^ value);
>> >> >> +
>> >> >> +  if (wi::sext (this->mask, this->precision) == -1)
>> >> >> +return set_to_bottom ();
>> >> >> +
>> >> >> +  bool changed = this->mask != old_mask;
>> >> >> +  return changed;
>> >> >> +}
>> >> >> +
>> >> >> +/* Meet the bits lattice with operand
>> >> >> +   described by > >> >> +
>> >> >> +bool
>> >> >> +ipcp_bits_lattice::meet_with (widest_int value, widest_int mask,
>> >> >> +   signop sgn, unsigned precision)
>> >> >> +{
>> >> >> +  if (bottom_p ())
>> >> >> +return false;
>> >> >> +
>> >> >> +  if (top_p ())
>> >> >> +{
>> >> >> +  if (wi::sext (mask, precision) == -1)
>> >> >> + return set_to_bottom ();
>> >> >> +  return set_to_constant (value, mask, sgn, precision);
>> >> >> +}
>> >> >> +
>> >> >> +  return meet_with_1 (value, mask);
>> >> >
>> >> > What if precisions do not match?
>> >> Sorry I don't understand. Since we extend to widest_int, precision
>> >> would be same ?
>> >
>> > I meant what if:
>> >
>> >   this->precision != precision /* the parameter value */
>> >
>> > (you see, giving both the same name is error-prone).  You are
>> > propagating the recorded precision gathered from types of the actual
>> > arguments in calls, those can be different.  For example, one caller
>> > can pass a direct integer value with full integer precision, another
>> > caller can pass in that same argument an enum value with a very
>> > limited precision.  Your code ignores that difference and the
>> > resulting precision is the one that arrives first.  If it is the 

Re: [RFC] ipa bitwise constant propagation

[Martin Jambor]

Hi,

On Tue, Aug 09, 2016 at 01:41:21PM +0530, Prathamesh Kulkarni wrote:
> On 8 August 2016 at 19:33, Martin Jambor  wrote:
> >> >> +class ipcp_bits_lattice
> >> >> +{
> >> >> +public:
> >> >> +  bool bottom_p () { return lattice_val == IPA_BITS_VARYING; }
> >> >> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
> >> >> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
> >> >> +  bool set_to_bottom ();
> >> >> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
> >> >> +
> >> >> +  widest_int get_value () { return value; }
> >> >> +  widest_int get_mask () { return mask; }
> >> >> +  signop get_sign () { return sgn; }
> >> >> +  unsigned get_precision () { return precision; }
> >> >> +
> >> >> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
> >> >> +  bool meet_with (widest_int, widest_int, signop, unsigned);
> >> >> +
> >> >> +  void print (FILE *);
> >> >> +
> >> >> +private:
> >> >> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
> >> >> lattice_val;
> >> >> +  widest_int value, mask;
> >> >> +  signop sgn;
> >> >> +  unsigned precision;
> >
> > I know that the existing code in ipa-cp.c does not do this, but please
> > prefix member variables with "m_" like our coding style guidelines
> > suggest (or even require?).  You routinely reuse those same names in
> > names of parameters of meet_with and I believe that is a practice that
> > will sooner or later lead to confusing the two and bugs.
> Sorry about this, will change to m_ prefix in followup patch.

Thanks a lot.

> >
> >> >> +
> >> >> +  bool meet_with_1 (widest_int, widest_int);
> >> >> +  void get_value_and_mask (tree, widest_int *, widest_int *);
> >> >> +};
> >> >> +
> >> >>  /* Structure containing lattices for a parameter itself and for pieces 
> >> >> of
> >> >> aggregates that are passed in the parameter or by a reference in a 
> >> >> parameter
> >> >> plus some other useful flags.  */
> >> >> @@ -281,6 +316,8 @@ public:
> >> >>ipcp_agg_lattice *aggs;
> >> >>/* Lattice describing known alignment.  */
> >> >>ipcp_alignment_lattice alignment;
> >> >> +  /* Lattice describing known bits.  */
> >> >> +  ipcp_bits_lattice bits_lattice;
> >> >>/* Number of aggregate lattices */
> >> >>int aggs_count;
> >> >>/* True if aggregate data were passed by reference (as opposed to by
> >> >> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
> >> >>  fprintf (f, " Alignment %u, misalignment %u\n", align, 
> >> >> misalign);
> >> >>  }
> >> >>
> >>
> >> ...
> >>
> >> >> +}
> >> >> +
> >> >> +/* Meet operation, similar to ccp_lattice_meet, we xor values
> >> >> +   if this->value, value have different values at same bit positions, 
> >> >> we want
> >> >> +   to drop that bit to varying. Return true if mask is changed.
> >> >> +   This function assumes that the lattice value is in CONSTANT state  
> >> >> */
> >> >> +
> >> >> +bool
> >> >> +ipcp_bits_lattice::meet_with_1 (widest_int value, widest_int mask)
> >> >> +{
> >> >> +  gcc_assert (constant_p ());
> >> >> +
> >> >> +  widest_int old_mask = this->mask;
> >> >> +  this->mask = (this->mask | mask) | (this->value ^ value);
> >> >> +
> >> >> +  if (wi::sext (this->mask, this->precision) == -1)
> >> >> +return set_to_bottom ();
> >> >> +
> >> >> +  bool changed = this->mask != old_mask;
> >> >> +  return changed;
> >> >> +}
> >> >> +
> >> >> +/* Meet the bits lattice with operand
> >> >> +   described by  >> >> +
> >> >> +bool
> >> >> +ipcp_bits_lattice::meet_with (widest_int value, widest_int mask,
> >> >> +   signop sgn, unsigned precision)
> >> >> +{
> >> >> +  if (bottom_p ())
> >> >> +return false;
> >> >> +
> >> >> +  if (top_p ())
> >> >> +{
> >> >> +  if (wi::sext (mask, precision) == -1)
> >> >> + return set_to_bottom ();
> >> >> +  return set_to_constant (value, mask, sgn, precision);
> >> >> +}
> >> >> +
> >> >> +  return meet_with_1 (value, mask);
> >> >
> >> > What if precisions do not match?
> >> Sorry I don't understand. Since we extend to widest_int, precision
> >> would be same ?
> >
> > I meant what if:
> >
> >   this->precision != precision /* the parameter value */
> >
> > (you see, giving both the same name is error-prone).  You are
> > propagating the recorded precision gathered from types of the actual
> > arguments in calls, those can be different.  For example, one caller
> > can pass a direct integer value with full integer precision, another
> > caller can pass in that same argument an enum value with a very
> > limited precision.  Your code ignores that difference and the
> > resulting precision is the one that arrives first.  If it is the enum,
> > it might be too small for the integer value from the other call-site?
> Ah indeed the patch incorrectly propagates precision of argument.
> So IIUC in ipcp_bits_lattice, we want 

Re: [RFC] ipa bitwise constant propagation

[Richard Biener]

On Tue, 9 Aug 2016, Prathamesh Kulkarni wrote:

> On 8 August 2016 at 19:33, Martin Jambor  wrote:
> > Hi,
> >
> > thanks for following through.  You'll need an approval from Honza, but
> > I think the code looks good (I have looked at the places that I
> > believe have changed since the last week).  However, I have discovered
> > one new thing I don't like and still believe you need to handle
> > different precisions in lattice need:
> >
> > On Mon, Aug 08, 2016 at 03:08:35AM +0530, Prathamesh Kulkarni wrote:
> >> On 5 August 2016 at 18:06, Martin Jambor  wrote:
> >>
> >> ...
> >>
> >> >> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> >> >> index 5b6cb9a..b770f6a 100644
> >> >> --- a/gcc/ipa-cp.c
> >> >> +++ b/gcc/ipa-cp.c
> >> >> @@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If not see
> >> >>  #include "params.h"
> >> >>  #include "ipa-inline.h"
> >> >>  #include "ipa-utils.h"
> >> >> +#include "tree-ssa-ccp.h"
> >> >>
> >> >>  template  class ipcp_value;
> >> >>
> >> >> @@ -266,6 +267,40 @@ private:
> >> >>bool meet_with_1 (unsigned new_align, unsigned new_misalign);
> >> >>  };
> >> >>
> >> >> +/* Lattice of known bits, only capable of holding one value.
> >> >> +   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
> >> >> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
> >> >> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
> >> >> +  bool set_to_bottom ();
> >> >> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
> >> >> +
> >> >> +  widest_int get_value () { return value; }
> >> >> +  widest_int get_mask () { return mask; }
> >> >> +  signop get_sign () { return sgn; }
> >> >> +  unsigned get_precision () { return precision; }
> >> >> +
> >> >> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
> >> >> +  bool meet_with (widest_int, widest_int, signop, unsigned);
> >> >> +
> >> >> +  void print (FILE *);
> >> >> +
> >> >> +private:
> >> >> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
> >> >> lattice_val;
> >> >> +  widest_int value, mask;
> >> >> +  signop sgn;
> >> >> +  unsigned precision;
> >
> > I know that the existing code in ipa-cp.c does not do this, but please
> > prefix member variables with "m_" like our coding style guidelines
> > suggest (or even require?).  You routinely reuse those same names in
> > names of parameters of meet_with and I believe that is a practice that
> > will sooner or later lead to confusing the two and bugs.
> Sorry about this, will change to m_ prefix in followup patch.
> >
> >> >> +
> >> >> +  bool meet_with_1 (widest_int, widest_int);
> >> >> +  void get_value_and_mask (tree, widest_int *, widest_int *);
> >> >> +};
> >> >> +
> >> >>  /* Structure containing lattices for a parameter itself and for pieces 
> >> >> of
> >> >> aggregates that are passed in the parameter or by a reference in a 
> >> >> parameter
> >> >> plus some other useful flags.  */
> >> >> @@ -281,6 +316,8 @@ public:
> >> >>ipcp_agg_lattice *aggs;
> >> >>/* Lattice describing known alignment.  */
> >> >>ipcp_alignment_lattice alignment;
> >> >> +  /* Lattice describing known bits.  */
> >> >> +  ipcp_bits_lattice bits_lattice;
> >> >>/* Number of aggregate lattices */
> >> >>int aggs_count;
> >> >>/* True if aggregate data were passed by reference (as opposed to by
> >> >> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
> >> >>  fprintf (f, " Alignment %u, misalignment %u\n", align, 
> >> >> misalign);
> >> >>  }
> >> >>
> >>
> >> ...
> >>
> >> >> +/* Convert operand to value, mask form.  */
> >> >> +
> >> >> +void
> >> >> +ipcp_bits_lattice::get_value_and_mask (tree operand, widest_int 
> >> >> *valuep, widest_int *maskp)
> >> >> +{
> >> >> +  wide_int get_nonzero_bits (const_tree);
> >> >> +
> >> >> +  if (TREE_CODE (operand) == INTEGER_CST)
> >> >> +{
> >> >> +  *valuep = wi::to_widest (operand);
> >> >> +  *maskp = 0;
> >> >> +}
> >> >> +  else if (TREE_CODE (operand) == SSA_NAME)
> >> >
> >> > IIUC, operand is the operand from pass-through jump function and that
> >> > should never be an SSA_NAME.  I have even looked at how we generate
> >> > them and it seems fairly safe to say that they never are.  If you have
> >> > seen an SSA_NAME here, it is a bug and please let me know because
> >> > sooner or later it will cause an assert.
> >> >
> >> >> +{
> >> >> +  *valuep = 0;
> >> >> +  *maskp = widest_int::from (get_nonzero_bits (operand), UNSIGNED);
> >> >> +}
> >> >> +  else
> >> >> +gcc_unreachable ();
> >> >
> >> > The operand however can be any any other 

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 8 August 2016 at 19:33, Martin Jambor  wrote:
> Hi,
>
> thanks for following through.  You'll need an approval from Honza, but
> I think the code looks good (I have looked at the places that I
> believe have changed since the last week).  However, I have discovered
> one new thing I don't like and still believe you need to handle
> different precisions in lattice need:
>
> On Mon, Aug 08, 2016 at 03:08:35AM +0530, Prathamesh Kulkarni wrote:
>> On 5 August 2016 at 18:06, Martin Jambor  wrote:
>>
>> ...
>>
>> >> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> >> index 5b6cb9a..b770f6a 100644
>> >> --- a/gcc/ipa-cp.c
>> >> +++ b/gcc/ipa-cp.c
>> >> @@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If not see
>> >>  #include "params.h"
>> >>  #include "ipa-inline.h"
>> >>  #include "ipa-utils.h"
>> >> +#include "tree-ssa-ccp.h"
>> >>
>> >>  template  class ipcp_value;
>> >>
>> >> @@ -266,6 +267,40 @@ private:
>> >>bool meet_with_1 (unsigned new_align, unsigned new_misalign);
>> >>  };
>> >>
>> >> +/* Lattice of known bits, only capable of holding one value.
>> >> +   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
>> >> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
>> >> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
>> >> +  bool set_to_bottom ();
>> >> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
>> >> +
>> >> +  widest_int get_value () { return value; }
>> >> +  widest_int get_mask () { return mask; }
>> >> +  signop get_sign () { return sgn; }
>> >> +  unsigned get_precision () { return precision; }
>> >> +
>> >> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
>> >> +  bool meet_with (widest_int, widest_int, signop, unsigned);
>> >> +
>> >> +  void print (FILE *);
>> >> +
>> >> +private:
>> >> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
>> >> lattice_val;
>> >> +  widest_int value, mask;
>> >> +  signop sgn;
>> >> +  unsigned precision;
>
> I know that the existing code in ipa-cp.c does not do this, but please
> prefix member variables with "m_" like our coding style guidelines
> suggest (or even require?).  You routinely reuse those same names in
> names of parameters of meet_with and I believe that is a practice that
> will sooner or later lead to confusing the two and bugs.
Sorry about this, will change to m_ prefix in followup patch.
>
>> >> +
>> >> +  bool meet_with_1 (widest_int, widest_int);
>> >> +  void get_value_and_mask (tree, widest_int *, widest_int *);
>> >> +};
>> >> +
>> >>  /* Structure containing lattices for a parameter itself and for pieces of
>> >> aggregates that are passed in the parameter or by a reference in a 
>> >> parameter
>> >> plus some other useful flags.  */
>> >> @@ -281,6 +316,8 @@ public:
>> >>ipcp_agg_lattice *aggs;
>> >>/* Lattice describing known alignment.  */
>> >>ipcp_alignment_lattice alignment;
>> >> +  /* Lattice describing known bits.  */
>> >> +  ipcp_bits_lattice bits_lattice;
>> >>/* Number of aggregate lattices */
>> >>int aggs_count;
>> >>/* True if aggregate data were passed by reference (as opposed to by
>> >> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
>> >>  fprintf (f, " Alignment %u, misalignment %u\n", align, 
>> >> misalign);
>> >>  }
>> >>
>>
>> ...
>>
>> >> +/* Convert operand to value, mask form.  */
>> >> +
>> >> +void
>> >> +ipcp_bits_lattice::get_value_and_mask (tree operand, widest_int *valuep, 
>> >> widest_int *maskp)
>> >> +{
>> >> +  wide_int get_nonzero_bits (const_tree);
>> >> +
>> >> +  if (TREE_CODE (operand) == INTEGER_CST)
>> >> +{
>> >> +  *valuep = wi::to_widest (operand);
>> >> +  *maskp = 0;
>> >> +}
>> >> +  else if (TREE_CODE (operand) == SSA_NAME)
>> >
>> > IIUC, operand is the operand from pass-through jump function and that
>> > should never be an SSA_NAME.  I have even looked at how we generate
>> > them and it seems fairly safe to say that they never are.  If you have
>> > seen an SSA_NAME here, it is a bug and please let me know because
>> > sooner or later it will cause an assert.
>> >
>> >> +{
>> >> +  *valuep = 0;
>> >> +  *maskp = widest_int::from (get_nonzero_bits (operand), UNSIGNED);
>> >> +}
>> >> +  else
>> >> +gcc_unreachable ();
>> >
>> > The operand however can be any any other is_gimple_ip_invariant tree.
>> > I assume that you could hit this gcc_unreachable only in a program
>> > with undefined behavior (or with a Fortran CONST_DECL?) but you should
>> > not ICE here.
>> Changed to:
>> if (TREE_CODE (operand) == INTEGER_CST)
>> {
>>   *valuep = wi::to_widest (operand);
>>  

Re: [RFC] ipa bitwise constant propagation

[David Malcolm]

On Mon, 2016-08-08 at 16:03 +0200, Martin Jambor wrote:
> Hi,
> 
> thanks for following through.  You'll need an approval from Honza,
> but
> I think the code looks good (I have looked at the places that I
> believe have changed since the last week).  However, I have
> discovered
> one new thing I don't like and still believe you need to handle
> different precisions in lattice need:
> 
> On Mon, Aug 08, 2016 at 03:08:35AM +0530, Prathamesh Kulkarni wrote:
> > On 5 August 2016 at 18:06, Martin Jambor  wrote:
> > 
> > ...
> > 
> > > > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> > > > index 5b6cb9a..b770f6a 100644
> > > > --- a/gcc/ipa-cp.c
> > > > +++ b/gcc/ipa-cp.c
> > > > @@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If
> > > > not see
> > > >  #include "params.h"
> > > >  #include "ipa-inline.h"
> > > >  #include "ipa-utils.h"
> > > > +#include "tree-ssa-ccp.h"
> > > > 
> > > >  template  class ipcp_value;
> > > > 
> > > > @@ -266,6 +267,40 @@ private:
> > > >bool meet_with_1 (unsigned new_align, unsigned
> > > > new_misalign);
> > > >  };
> > > > 
> > > > +/* Lattice of known bits, only capable of holding one value.
> > > > +   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
> > > > +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
> > > > +  bool constant_p () { return lattice_val ==
> > > > IPA_BITS_CONSTANT; }
> > > > +  bool set_to_bottom ();
> > > > +  bool set_to_constant (widest_int, widest_int, signop,
> > > > unsigned);
> > > > +
> > > > +  widest_int get_value () { return value; }
> > > > +  widest_int get_mask () { return mask; }
> > > > +  signop get_sign () { return sgn; }
> > > > +  unsigned get_precision () { return precision; }
> > > > +
> > > > +  bool meet_with (ipcp_bits_lattice& other, enum tree_code,
> > > > tree);
> > > > +  bool meet_with (widest_int, widest_int, signop, unsigned);
> > > > +
> > > > +  void print (FILE *);
> > > > +
> > > > +private:
> > > > +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT,
> > > > IPA_BITS_VARYING } lattice_val;
> > > > +  widest_int value, mask;
> > > > +  signop sgn;
> > > > +  unsigned precision;
> 
> I know that the existing code in ipa-cp.c does not do this, but
> please
> prefix member variables with "m_" like our coding style guidelines
> suggest (or even require?).  You routinely reuse those same names in
> names of parameters of meet_with and I believe that is a practice
> that
> will sooner or later lead to confusing the two and bugs.

I'm not a reviewer, and not very familiar with this code, but is it
possible to add a couple of examples to the descriptive comment of
 class ipcp_bits_lattice?  I'm finding it hard to understand how the
various fields interact, in particular "value" and "mask" interact (or
rather "m_value" and "m_mask").  I think a concrete example would make
things much clearer.  This thread talked about this below...

[...]

> > > It is probably just me not being particularly sharp on a Friday
> > > afternoon and I might not understand the semantics of mask well
> > > (also,
> > > you did not document it :-), but... assume that we are looking at
> > > a
> > > binary and operation, other comes from an SSA pointer and its
> > > mask
> > > would be binary 100 and its value 0 because that's what you set
> > > for
> > > ssa names in ipa-prop.h, and the operand is binary value 101,
> > > which
> > > means that get_value_and_mask returns mask 0 and value 101.  Now,
> > > bit_value_binop_1 would return value 0 & 101 = 0 and mask
> > > according to
> > > 
> > > (m1 | m2) & ((v1 | m1) & (v2 | m2))
> > > 
> > > so in our case
> > > 
> > > (100b & 0) & ((0 | 100b) & (101b | 0)) = 0 & 100b = 0.
> > Shouldn't this be:
> > (100b | 0) & ((0 | 100b) & (101b | 0)) = 100 & 100 = 100 -;)
> 
> Eh, right, sorry.  I just find the term mask confusing when we do not
> actually mask anything with it (but I guess it is good to be
> consistent so let's keep it).

...so presumably it would be good to capture something like that within
the descriptive comment of the class.

[...]

Hope this is constructive
Dave

Re: [RFC] ipa bitwise constant propagation

[Martin Jambor]

Hi,

thanks for following through.  You'll need an approval from Honza, but
I think the code looks good (I have looked at the places that I
believe have changed since the last week).  However, I have discovered
one new thing I don't like and still believe you need to handle
different precisions in lattice need:

On Mon, Aug 08, 2016 at 03:08:35AM +0530, Prathamesh Kulkarni wrote:
> On 5 August 2016 at 18:06, Martin Jambor  wrote:
>
> ...
>
> >> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> >> index 5b6cb9a..b770f6a 100644
> >> --- a/gcc/ipa-cp.c
> >> +++ b/gcc/ipa-cp.c
> >> @@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If not see
> >>  #include "params.h"
> >>  #include "ipa-inline.h"
> >>  #include "ipa-utils.h"
> >> +#include "tree-ssa-ccp.h"
> >>
> >>  template  class ipcp_value;
> >>
> >> @@ -266,6 +267,40 @@ private:
> >>bool meet_with_1 (unsigned new_align, unsigned new_misalign);
> >>  };
> >>
> >> +/* Lattice of known bits, only capable of holding one value.
> >> +   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
> >> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
> >> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
> >> +  bool set_to_bottom ();
> >> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
> >> +
> >> +  widest_int get_value () { return value; }
> >> +  widest_int get_mask () { return mask; }
> >> +  signop get_sign () { return sgn; }
> >> +  unsigned get_precision () { return precision; }
> >> +
> >> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
> >> +  bool meet_with (widest_int, widest_int, signop, unsigned);
> >> +
> >> +  void print (FILE *);
> >> +
> >> +private:
> >> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
> >> lattice_val;
> >> +  widest_int value, mask;
> >> +  signop sgn;
> >> +  unsigned precision;

I know that the existing code in ipa-cp.c does not do this, but please
prefix member variables with "m_" like our coding style guidelines
suggest (or even require?).  You routinely reuse those same names in
names of parameters of meet_with and I believe that is a practice that
will sooner or later lead to confusing the two and bugs.

> >> +
> >> +  bool meet_with_1 (widest_int, widest_int);
> >> +  void get_value_and_mask (tree, widest_int *, widest_int *);
> >> +};
> >> +
> >>  /* Structure containing lattices for a parameter itself and for pieces of
> >> aggregates that are passed in the parameter or by a reference in a 
> >> parameter
> >> plus some other useful flags.  */
> >> @@ -281,6 +316,8 @@ public:
> >>ipcp_agg_lattice *aggs;
> >>/* Lattice describing known alignment.  */
> >>ipcp_alignment_lattice alignment;
> >> +  /* Lattice describing known bits.  */
> >> +  ipcp_bits_lattice bits_lattice;
> >>/* Number of aggregate lattices */
> >>int aggs_count;
> >>/* True if aggregate data were passed by reference (as opposed to by
> >> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
> >>  fprintf (f, " Alignment %u, misalignment %u\n", align, 
> >> misalign);
> >>  }
> >>
>
> ...
>
> >> +/* Convert operand to value, mask form.  */
> >> +
> >> +void
> >> +ipcp_bits_lattice::get_value_and_mask (tree operand, widest_int *valuep, 
> >> widest_int *maskp)
> >> +{
> >> +  wide_int get_nonzero_bits (const_tree);
> >> +
> >> +  if (TREE_CODE (operand) == INTEGER_CST)
> >> +{
> >> +  *valuep = wi::to_widest (operand);
> >> +  *maskp = 0;
> >> +}
> >> +  else if (TREE_CODE (operand) == SSA_NAME)
> >
> > IIUC, operand is the operand from pass-through jump function and that
> > should never be an SSA_NAME.  I have even looked at how we generate
> > them and it seems fairly safe to say that they never are.  If you have
> > seen an SSA_NAME here, it is a bug and please let me know because
> > sooner or later it will cause an assert.
> >
> >> +{
> >> +  *valuep = 0;
> >> +  *maskp = widest_int::from (get_nonzero_bits (operand), UNSIGNED);
> >> +}
> >> +  else
> >> +gcc_unreachable ();
> >
> > The operand however can be any any other is_gimple_ip_invariant tree.
> > I assume that you could hit this gcc_unreachable only in a program
> > with undefined behavior (or with a Fortran CONST_DECL?) but you should
> > not ICE here.
> Changed to:
> if (TREE_CODE (operand) == INTEGER_CST)
> {
>   *valuep = wi::to_widest (operand);
>   *maskp = 0;
> }
>   else
> {
>   *valuep = 0;
>   *maskp = -1;
> }
> 
> I am not sure how to extract nonzero bits for gimple_ip_invariant if
> it's not INTEGER_CST,

I don't think that you reasonably can.

> so setting to unknown (value = 0, 

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 5 August 2016 at 18:06, Martin Jambor  wrote:
> Hi,
Hi Martin,
Thanks for the review. Please find my responses inline.
>
> generally speaking, the ipa-cp.c and ipa-cp.[hc] bits look reasonable,
> but I have a few comments:
>
> On Thu, Aug 04, 2016 at 12:06:18PM +0530, Prathamesh Kulkarni wrote:
>> Hi,
>> This is a prototype patch for propagating known/unknown bits 
>> inter-procedurally.
>> for integral types which propagates info obtained from get_nonzero_bits ().
>>
>> Patch required making following changes:
>> a) To make info from get_nonzero_bits() available to ipa
>
> in IPA phase, you should not be looking at SSA_NAMEs, those will not
> be available with LTO when you do not have access to function bodies
> and thus also not to SSA_NAMES.  In IPA, you should only rely on hat
> you have in jump functions.
>
>> , I had to remove
>> guard !nonzero_p in ccp_finalize. However that triggered the following ICE
>> in get_ptr_info() for default_none.f95 (and several other fortran tests)
>> with options: -fopenacc -O2
>> ICE: http://pastebin.com/KjD7HMQi
>> I confirmed with Richard that this was a latent issue.
>>
>>
>> I suppose we would want to gate this on some flag, say -fipa-bit-cp ?
>
> Yes, definitely.
Added -fipa-cp-bit (mirroring -fipa-cp-alignment)
>
>> I haven't yet gated it on the flag, will do in next version of patch.
>> I have added some very simple test-cases, I will try to add more
>> meaningful ones.
>
>
>>
>> Patch passes bootstrap+test on x86_64-unknown-linux-gnu
>> and cross-tested on arm*-*-* and aarch64*-*-* with the exception
>> of some fortran tests failing due to above ICE.
>>
>> As next steps, I am planning to extend it to handle alignment propagation,
>> and do further testing (lto-bootstrap, chromium).
>> I would be grateful for feedback on the current patch.
>>
>> Thanks,
>> Prathamesh
>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index 5b6cb9a..b770f6a 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "params.h"
>>  #include "ipa-inline.h"
>>  #include "ipa-utils.h"
>> +#include "tree-ssa-ccp.h"
>>
>>  template  class ipcp_value;
>>
>> @@ -266,6 +267,40 @@ private:
>>bool meet_with_1 (unsigned new_align, unsigned new_misalign);
>>  };
>>
>> +/* Lattice of known bits, only capable of holding one value.
>> +   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
>> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
>> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
>> +  bool set_to_bottom ();
>> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
>> +
>> +  widest_int get_value () { return value; }
>> +  widest_int get_mask () { return mask; }
>> +  signop get_sign () { return sgn; }
>> +  unsigned get_precision () { return precision; }
>> +
>> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
>> +  bool meet_with (widest_int, widest_int, signop, unsigned);
>> +
>> +  void print (FILE *);
>> +
>> +private:
>> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
>> lattice_val;
>> +  widest_int value, mask;
>> +  signop sgn;
>> +  unsigned precision;
>> +
>> +  bool meet_with_1 (widest_int, widest_int);
>> +  void get_value_and_mask (tree, widest_int *, widest_int *);
>> +};
>> +
>>  /* Structure containing lattices for a parameter itself and for pieces of
>> aggregates that are passed in the parameter or by a reference in a 
>> parameter
>> plus some other useful flags.  */
>> @@ -281,6 +316,8 @@ public:
>>ipcp_agg_lattice *aggs;
>>/* Lattice describing known alignment.  */
>>ipcp_alignment_lattice alignment;
>> +  /* Lattice describing known bits.  */
>> +  ipcp_bits_lattice bits_lattice;
>>/* Number of aggregate lattices */
>>int aggs_count;
>>/* True if aggregate data were passed by reference (as opposed to by
>> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
>>  fprintf (f, " Alignment %u, misalignment %u\n", align, 
>> misalign);
>>  }
>>
>> +void
>> +ipcp_bits_lattice::print (FILE *f)
>> +{
>> +  if (top_p ())
>> +fprintf (f, " Bits unknown (TOP)\n");
>> +  else if (bottom_p ())
>> +fprintf (f, " Bits unusable (BOTTOM)\n");
>> +  else
>> +{
>> +  fprintf (f, " Bits: value = "); print_hex (get_value (), f);
>> +  fprintf (f, ", mask = "); print_hex (get_mask (), f);
>> +  fprintf (f, "\n");
>> +}
>> +}
>> +
>>  /* Print all ipcp_lattices of all functions to F.  */
>>
>>  static void
>> @@ -484,6 +536,7 @@ print_all_lattices (FILE * f, bool dump_sources, bool 
>> dump_benefits)
>> fprintf (f, " ctxs: ");
>> 

Re: [RFC] ipa bitwise constant propagation

[Martin Jambor]

Hi,

generally speaking, the ipa-cp.c and ipa-cp.[hc] bits look reasonable,
but I have a few comments:

On Thu, Aug 04, 2016 at 12:06:18PM +0530, Prathamesh Kulkarni wrote:
> Hi,
> This is a prototype patch for propagating known/unknown bits 
> inter-procedurally.
> for integral types which propagates info obtained from get_nonzero_bits ().
> 
> Patch required making following changes:
> a) To make info from get_nonzero_bits() available to ipa

in IPA phase, you should not be looking at SSA_NAMEs, those will not
be available with LTO when you do not have access to function bodies
and thus also not to SSA_NAMES.  In IPA, you should only rely on hat
you have in jump functions.

> , I had to remove
> guard !nonzero_p in ccp_finalize. However that triggered the following ICE
> in get_ptr_info() for default_none.f95 (and several other fortran tests)
> with options: -fopenacc -O2
> ICE: http://pastebin.com/KjD7HMQi
> I confirmed with Richard that this was a latent issue.
> 
> 
> I suppose we would want to gate this on some flag, say -fipa-bit-cp ?

Yes, definitely.

> I haven't yet gated it on the flag, will do in next version of patch.
> I have added some very simple test-cases, I will try to add more
> meaningful ones.


> 
> Patch passes bootstrap+test on x86_64-unknown-linux-gnu
> and cross-tested on arm*-*-* and aarch64*-*-* with the exception
> of some fortran tests failing due to above ICE.
> 
> As next steps, I am planning to extend it to handle alignment propagation,
> and do further testing (lto-bootstrap, chromium).
> I would be grateful for feedback on the current patch.
> 
> Thanks,
> Prathamesh

> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 5b6cb9a..b770f6a 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "params.h"
>  #include "ipa-inline.h"
>  #include "ipa-utils.h"
> +#include "tree-ssa-ccp.h"
>  
>  template  class ipcp_value;
>  
> @@ -266,6 +267,40 @@ private:
>bool meet_with_1 (unsigned new_align, unsigned new_misalign);
>  };
>  
> +/* Lattice of known bits, only capable of holding one value.
> +   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
> +  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
> +  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
> +  bool set_to_bottom ();
> +  bool set_to_constant (widest_int, widest_int, signop, unsigned);
> + 
> +  widest_int get_value () { return value; }
> +  widest_int get_mask () { return mask; }
> +  signop get_sign () { return sgn; }
> +  unsigned get_precision () { return precision; }
> +
> +  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
> +  bool meet_with (widest_int, widest_int, signop, unsigned);
> +
> +  void print (FILE *);
> +
> +private:
> +  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } 
> lattice_val;
> +  widest_int value, mask;
> +  signop sgn;
> +  unsigned precision;
> +
> +  bool meet_with_1 (widest_int, widest_int); 
> +  void get_value_and_mask (tree, widest_int *, widest_int *);
> +}; 
> +
>  /* Structure containing lattices for a parameter itself and for pieces of
> aggregates that are passed in the parameter or by a reference in a 
> parameter
> plus some other useful flags.  */
> @@ -281,6 +316,8 @@ public:
>ipcp_agg_lattice *aggs;
>/* Lattice describing known alignment.  */
>ipcp_alignment_lattice alignment;
> +  /* Lattice describing known bits.  */
> +  ipcp_bits_lattice bits_lattice;
>/* Number of aggregate lattices */
>int aggs_count;
>/* True if aggregate data were passed by reference (as opposed to by
> @@ -458,6 +495,21 @@ ipcp_alignment_lattice::print (FILE * f)
>  fprintf (f, " Alignment %u, misalignment %u\n", align, misalign);
>  }
>  
> +void
> +ipcp_bits_lattice::print (FILE *f)
> +{
> +  if (top_p ())
> +fprintf (f, " Bits unknown (TOP)\n");
> +  else if (bottom_p ())
> +fprintf (f, " Bits unusable (BOTTOM)\n");
> +  else
> +{
> +  fprintf (f, " Bits: value = "); print_hex (get_value (), f);
> +  fprintf (f, ", mask = "); print_hex (get_mask (), f);
> +  fprintf (f, "\n");
> +}
> +}
> +
>  /* Print all ipcp_lattices of all functions to F.  */
>  
>  static void
> @@ -484,6 +536,7 @@ print_all_lattices (FILE * f, bool dump_sources, bool 
> dump_benefits)
> fprintf (f, " ctxs: ");
> plats->ctxlat.print (f, dump_sources, dump_benefits);
> plats->alignment.print (f);
> +   plats->bits_lattice.print (f);
> if (plats->virt_call)
>   fprintf (f, "virt_call flag set\n");
>  
> @@ -911,6 +964,161 @@ ipcp_alignment_lattice::meet_with (const 
> 

Re: [RFC] ipa bitwise constant propagation

[Jan Hubicka]

> Hi Honza,
> 
> On 04/08/16 23:05, Jan Hubicka wrote:
> >>I didn't look at the propagation part but eventually the IPA-CP
> >>lattice gets quite big.  Also the alignment lattice is very
> >>similar to the bits lattice so why not merge those two?  But
> >
> >This was always the original idea to replace alignment propagation by bitwise
> >ccp.  I suppose we only have issue here because nonzero bits are not tracked 
> >for
> >pointers so we need to feed the original lattices by hand?
> 
> I also raised this one with Prathamesh off line. With the early-vrp,
> we should have nonzero_bits for non pointers. For pointers we should
> feed the lattices with get_pointer_alignment_1 as it is done in
> ipa-cpalignment propagation.

Yes, that is the general idea. Note that also for pointers it would be
very useful to track what pointers are non-NULL (C++ multiple inheritance 
inserts
a lot of NULL pointer checks that confuse us in later analysis and it would
be nice to optimize them out). I am not very convinced saving a pointer is
worth to make difference between pointers/nonpointers for all the local
tracking.
> 
> >We could also make use of VR ranges and bits while evaultaing predicates
> >in ipa-inline-analysis. I can look into it after returning from Leeds.
> 
> Indeed. With ealrly dom based VRP (non iterative at this point),
> some of the ranges can be pessimistic and can impact the estimation.
> Let me have a look at this.

Yes, but those are independent issues - size/time estimates should
take into account the new info we have and we should work on getting
it better when we can ;)

I will try to revisit the size/time code after returning from
my vacation and turn it into sreals for time + cleanup/generalize the APIs
a bit. I tried to do it last stage1 but got stuck on some of ugly side cases
+ gengtype not liking sreal type.

Honza
> 
> Thanks,
> Kugan
> 

Re: [RFC] ipa bitwise constant propagation

[kugan]

Hi Honza,

On 04/08/16 23:05, Jan Hubicka wrote:

I didn't look at the propagation part but eventually the IPA-CP
lattice gets quite big.  Also the alignment lattice is very
similar to the bits lattice so why not merge those two?  But


This was always the original idea to replace alignment propagation by bitwise
ccp.  I suppose we only have issue here because nonzero bits are not tracked for
pointers so we need to feed the original lattices by hand?


I also raised this one with Prathamesh off line. With the early-vrp, we 
should have nonzero_bits for non pointers. For pointers we should feed 
the lattices with get_pointer_alignment_1 as it is done in 
ipa-cpalignment propagation.



We could also make use of VR ranges and bits while evaultaing predicates
in ipa-inline-analysis. I can look into it after returning from Leeds.


Indeed. With ealrly dom based VRP (non iterative at this point), some of 
the ranges can be pessimistic and can impact the estimation. Let me have 
a look at this.


Thanks,
Kugan

Re: [RFC] ipa bitwise constant propagation

[Jan Hubicka]

> I didn't look at the propagation part but eventually the IPA-CP
> lattice gets quite big.  Also the alignment lattice is very
> similar to the bits lattice so why not merge those two?  But

This was always the original idea to replace alignment propagation by bitwise
ccp.  I suppose we only have issue here because nonzero bits are not tracked for
pointers so we need to feed the original lattices by hand?

We could also make use of VR ranges and bits while evaultaing predicates
in ipa-inline-analysis. I can look into it after returning from Leeds.

Honza
> in the end it's Martins/Honzas call here.  Note there is
> trailing_wide_ints <> which could be used to improve memory usage
> based on the underlying type.
> 
> Thanks,
> Richard.

Re: [RFC] ipa bitwise constant propagation

[Richard Biener]

On Thu, 4 Aug 2016, Prathamesh Kulkarni wrote:

> On 4 August 2016 at 13:31, Richard Biener  wrote:
> > On Thu, 4 Aug 2016, Prathamesh Kulkarni wrote:
> >
> >> Hi,
> >> This is a prototype patch for propagating known/unknown bits 
> >> inter-procedurally.
> >> for integral types which propagates info obtained from get_nonzero_bits ().
> >>
> >> Patch required making following changes:
> >> a) To make info from get_nonzero_bits() available to ipa, I had to remove
> >> guard !nonzero_p in ccp_finalize. However that triggered the following ICE
> >> in get_ptr_info() for default_none.f95 (and several other fortran tests)
> >> with options: -fopenacc -O2
> >> ICE: http://pastebin.com/KjD7HMQi
> >> I confirmed with Richard that this was a latent issue.
> >
> > Can you plase bootstrap/test the fix for this separately?  (doesn't
> > seem to be included in this patch btw)
> Well I don't have the fix available -;)

Oh, I thought it was obvious:

Index: gcc/tree-inline.c
===
--- gcc/tree-inline.c   (revision 239117)
+++ gcc/tree-inline.c   (working copy)
@@ -242,7 +242,8 @@ remap_ssa_name (tree name, copy_body_dat
   SSA_NAME_OCCURS_IN_ABNORMAL_PHI (new_tree)
= SSA_NAME_OCCURS_IN_ABNORMAL_PHI (name);
   /* At least IPA points-to info can be directly transferred.  */
-  if (id->src_cfun->gimple_df
+  if (POINTER_TYPE_P (TREE_TYPE (name))
+ && id->src_cfun->gimple_df
  && id->src_cfun->gimple_df->ipa_pta
  && (pi = SSA_NAME_PTR_INFO (name))
  && !pi->pt.anything)
@@ -274,7 +275,8 @@ remap_ssa_name (tree name, copy_body_dat
   SSA_NAME_OCCURS_IN_ABNORMAL_PHI (new_tree)
= SSA_NAME_OCCURS_IN_ABNORMAL_PHI (name);
   /* At least IPA points-to info can be directly transferred.  */
-  if (id->src_cfun->gimple_df
+  if (POINTER_TYPE_P (TREE_TYPE (name))
+ && id->src_cfun->gimple_df
  && id->src_cfun->gimple_df->ipa_pta
  && (pi = SSA_NAME_PTR_INFO (name))
  && !pi->pt.anything)

similarly range info could be transfered of course.

> >
> >> b) I chose widest_int for representing value, mask in ipcp_bits_lattice
> >> and correspondingly changed declarations for
> >> bit_value_unop_1/bit_value_binop_1 to take
> >> precision and sign instead of type (those are the only two fields that
> >> were used). Both these functions are exported by tree-ssa-ccp.h
> >> I hope that's ok ?
> >
> > That's ok, but please change the functions to overloads of
> > bit_value_binop / bit_value_unop to not export ugly _1 names.
> >
> > -  signop sgn = TYPE_SIGN (type);
> > -  int width = TYPE_PRECISION (type);
> > +  signop sgn = type_sgn;
> > +  int width = (int) type_precision;
> >
> > please adjust parameter names to get rid of those now unnecessary
> > locals (and make the precision parameter an 'int').
> >
> >> c) Changed streamer_read_wi/streamer_write_wi to non-static.
> >> Ah I see Kugan has submitted a patch for this, so I will drop this hunk.
> >
> > But he streams wide_int, not widest_int.  I followed up on his
> > patch.
> Oops, I got confused, sorry about that.
> >
> >> d) We have following in tree-ssa-ccp.c:get_default_value ():
> >>   if (flag_tree_bit_ccp)
> >> {
> >>   wide_int nonzero_bits = get_nonzero_bits (var);
> >>   if (nonzero_bits != -1)
> >> {
> >>   val.lattice_val = CONSTANT;
> >>   val.value = build_zero_cst (TREE_TYPE (var));
> >>   val.mask = extend_mask (nonzero_bits);
> >> }
> >>
> >> extend_mask() sets all upper bits to 1 in nonzero_bits, ie, varying
> >> in terms of bit-ccp.
> >> I suppose in tree-ccp we need to extend mask if var is parameter since we 
> >> don't
> >> know in advance what values it will receive from different callers and 
> >> mark all
> >> upper bits as 1 to be safe.
> >
> > Not sure, it seems to me that we can zero-extend for unsigned types
> > and sign-extend for signed types (if the "sign"-bit of nonzero_bits
> > is one it properly makes higher bits undefined).  Can you change
> > the code accordingly?  (simply give extend_mask a sign-op and use
> > that appropriately?)  Please split out this change so it can be
> > tested separately.
> >
> >> However I suppose with ipa, we can determine exactly which bits of
> >> parameter are constant and
> >> setting all upper bits to 1 will become unnecessary ?
> >>
> >> For example, consider following artificial test-case:
> >> int f(int x)
> >> {
> >>   if (x > 300)
> >> return 1;
> >>   else
> >> return 2;
> >> }
> >>
> >> int main(int argc, char **argv)
> >> {
> >>   return f(argc & 0xc) + f (argc & 0x3);
> >> }
> >>
> >> For x, the mask would be meet of:
> >> <0, 0xc> meet <0, 0x3> == (0x3 | 0xc) | (0 ^ 0) == 0xf
> >> and ipcp_update_bits() sets nonzero_bits for x to 0xf.
> >> However get_default_value then calls extend_mask (0xf), 

Re: [RFC] ipa bitwise constant propagation

[kugan]

On 04/08/16 18:57, Prathamesh Kulkarni wrote:

On 4 August 2016 at 13:31, Richard Biener  wrote:

On Thu, 4 Aug 2016, Prathamesh Kulkarni wrote:


Hi,
This is a prototype patch for propagating known/unknown bits inter-procedurally.
for integral types which propagates info obtained from get_nonzero_bits ().

Patch required making following changes:
a) To make info from get_nonzero_bits() available to ipa, I had to remove
guard !nonzero_p in ccp_finalize. However that triggered the following ICE
in get_ptr_info() for default_none.f95 (and several other fortran tests)
with options: -fopenacc -O2
ICE: http://pastebin.com/KjD7HMQi
I confirmed with Richard that this was a latent issue.


Can you plase bootstrap/test the fix for this separately?  (doesn't
seem to be included in this patch btw)

Well I don't have the fix available -;)


This looks like what I fixed in 
https://patchwork.ozlabs.org/patch/648662/. I will commit that soon.


Thanks,
Kugan

Re: [RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

On 4 August 2016 at 13:31, Richard Biener  wrote:
> On Thu, 4 Aug 2016, Prathamesh Kulkarni wrote:
>
>> Hi,
>> This is a prototype patch for propagating known/unknown bits 
>> inter-procedurally.
>> for integral types which propagates info obtained from get_nonzero_bits ().
>>
>> Patch required making following changes:
>> a) To make info from get_nonzero_bits() available to ipa, I had to remove
>> guard !nonzero_p in ccp_finalize. However that triggered the following ICE
>> in get_ptr_info() for default_none.f95 (and several other fortran tests)
>> with options: -fopenacc -O2
>> ICE: http://pastebin.com/KjD7HMQi
>> I confirmed with Richard that this was a latent issue.
>
> Can you plase bootstrap/test the fix for this separately?  (doesn't
> seem to be included in this patch btw)
Well I don't have the fix available -;)
>
>> b) I chose widest_int for representing value, mask in ipcp_bits_lattice
>> and correspondingly changed declarations for
>> bit_value_unop_1/bit_value_binop_1 to take
>> precision and sign instead of type (those are the only two fields that
>> were used). Both these functions are exported by tree-ssa-ccp.h
>> I hope that's ok ?
>
> That's ok, but please change the functions to overloads of
> bit_value_binop / bit_value_unop to not export ugly _1 names.
>
> -  signop sgn = TYPE_SIGN (type);
> -  int width = TYPE_PRECISION (type);
> +  signop sgn = type_sgn;
> +  int width = (int) type_precision;
>
> please adjust parameter names to get rid of those now unnecessary
> locals (and make the precision parameter an 'int').
>
>> c) Changed streamer_read_wi/streamer_write_wi to non-static.
>> Ah I see Kugan has submitted a patch for this, so I will drop this hunk.
>
> But he streams wide_int, not widest_int.  I followed up on his
> patch.
Oops, I got confused, sorry about that.
>
>> d) We have following in tree-ssa-ccp.c:get_default_value ():
>>   if (flag_tree_bit_ccp)
>> {
>>   wide_int nonzero_bits = get_nonzero_bits (var);
>>   if (nonzero_bits != -1)
>> {
>>   val.lattice_val = CONSTANT;
>>   val.value = build_zero_cst (TREE_TYPE (var));
>>   val.mask = extend_mask (nonzero_bits);
>> }
>>
>> extend_mask() sets all upper bits to 1 in nonzero_bits, ie, varying
>> in terms of bit-ccp.
>> I suppose in tree-ccp we need to extend mask if var is parameter since we 
>> don't
>> know in advance what values it will receive from different callers and mark 
>> all
>> upper bits as 1 to be safe.
>
> Not sure, it seems to me that we can zero-extend for unsigned types
> and sign-extend for signed types (if the "sign"-bit of nonzero_bits
> is one it properly makes higher bits undefined).  Can you change
> the code accordingly?  (simply give extend_mask a sign-op and use
> that appropriately?)  Please split out this change so it can be
> tested separately.
>
>> However I suppose with ipa, we can determine exactly which bits of
>> parameter are constant and
>> setting all upper bits to 1 will become unnecessary ?
>>
>> For example, consider following artificial test-case:
>> int f(int x)
>> {
>>   if (x > 300)
>> return 1;
>>   else
>> return 2;
>> }
>>
>> int main(int argc, char **argv)
>> {
>>   return f(argc & 0xc) + f (argc & 0x3);
>> }
>>
>> For x, the mask would be meet of:
>> <0, 0xc> meet <0, 0x3> == (0x3 | 0xc) | (0 ^ 0) == 0xf
>> and ipcp_update_bits() sets nonzero_bits for x to 0xf.
>> However get_default_value then calls extend_mask (0xf), resulting in
>> all upper bits
>> being set to 1 and consequently the condition if (x > 300) doesn't get 
>> folded.
>
> But then why would the code trying to optimize the comparison look at
> bits that are outside of the precision?  (where do we try to use this
> info?  I see that VRP misses to use nonzero bits if no range info
> is present - I suppose set_nonzero_bits misses to eventually adjust
> the range.
>
> That said, where is the folding code and why does it care for those
> "uninteresting" bits at all?
Well there is following in bit_value_binop_1 for case LT_EXPR / LE_EXPR:
/* If the most significant bits are not known we know nothing.  */
if (wi::neg_p (o1mask) || wi::neg_p (o2mask))
  break;

IIUC extend_mask extends all upper bits to 1, and we hit break and
thus not perform folding.
ccp2 dump shows:
Folding statement: if (x_2(D) > 300)
which is likely CONSTANT
Not folded

Instead if we extend based on signop, then the condition gets folded correctly:
Folding statement: if (x_2(D) > 300)
which is likely CONSTANT
Folding predicate x_2(D) > 300 to 0
gimple_simplified to if (0 != 0)
Folded into: if (0 != 0)

I thought it was unsafe for ccp to extend based on sign-op,
so I guarded that on DECL_SET_BY_IPA.
I will try to change extend_mask to extend the mask based on signop
and get rid of the flag.

I will address your other comments in follow-up patch.

Thanks,
Prathamesh
>
>> To 

Re: [RFC] ipa bitwise constant propagation

[Richard Biener]

On Thu, 4 Aug 2016, Prathamesh Kulkarni wrote:

> Hi,
> This is a prototype patch for propagating known/unknown bits 
> inter-procedurally.
> for integral types which propagates info obtained from get_nonzero_bits ().
> 
> Patch required making following changes:
> a) To make info from get_nonzero_bits() available to ipa, I had to remove
> guard !nonzero_p in ccp_finalize. However that triggered the following ICE
> in get_ptr_info() for default_none.f95 (and several other fortran tests)
> with options: -fopenacc -O2
> ICE: http://pastebin.com/KjD7HMQi
> I confirmed with Richard that this was a latent issue.

Can you plase bootstrap/test the fix for this separately?  (doesn't
seem to be included in this patch btw)

> b) I chose widest_int for representing value, mask in ipcp_bits_lattice
> and correspondingly changed declarations for
> bit_value_unop_1/bit_value_binop_1 to take
> precision and sign instead of type (those are the only two fields that
> were used). Both these functions are exported by tree-ssa-ccp.h
> I hope that's ok ?

That's ok, but please change the functions to overloads of
bit_value_binop / bit_value_unop to not export ugly _1 names.

-  signop sgn = TYPE_SIGN (type);
-  int width = TYPE_PRECISION (type);
+  signop sgn = type_sgn;
+  int width = (int) type_precision;

please adjust parameter names to get rid of those now unnecessary
locals (and make the precision parameter an 'int').

> c) Changed streamer_read_wi/streamer_write_wi to non-static.
> Ah I see Kugan has submitted a patch for this, so I will drop this hunk.

But he streams wide_int, not widest_int.  I followed up on his
patch.

> d) We have following in tree-ssa-ccp.c:get_default_value ():
>   if (flag_tree_bit_ccp)
> {
>   wide_int nonzero_bits = get_nonzero_bits (var);
>   if (nonzero_bits != -1)
> {
>   val.lattice_val = CONSTANT;
>   val.value = build_zero_cst (TREE_TYPE (var));
>   val.mask = extend_mask (nonzero_bits);
> }
> 
> extend_mask() sets all upper bits to 1 in nonzero_bits, ie, varying
> in terms of bit-ccp.
> I suppose in tree-ccp we need to extend mask if var is parameter since we 
> don't
> know in advance what values it will receive from different callers and mark 
> all
> upper bits as 1 to be safe.

Not sure, it seems to me that we can zero-extend for unsigned types
and sign-extend for signed types (if the "sign"-bit of nonzero_bits
is one it properly makes higher bits undefined).  Can you change
the code accordingly?  (simply give extend_mask a sign-op and use
that appropriately?)  Please split out this change so it can be
tested separately.

> However I suppose with ipa, we can determine exactly which bits of
> parameter are constant and
> setting all upper bits to 1 will become unnecessary ?
> 
> For example, consider following artificial test-case:
> int f(int x)
> {
>   if (x > 300)
> return 1;
>   else
> return 2;
> }
> 
> int main(int argc, char **argv)
> {
>   return f(argc & 0xc) + f (argc & 0x3);
> }
> 
> For x, the mask would be meet of:
> <0, 0xc> meet <0, 0x3> == (0x3 | 0xc) | (0 ^ 0) == 0xf
> and ipcp_update_bits() sets nonzero_bits for x to 0xf.
> However get_default_value then calls extend_mask (0xf), resulting in
> all upper bits
> being set to 1 and consequently the condition if (x > 300) doesn't get folded.

But then why would the code trying to optimize the comparison look at
bits that are outside of the precision?  (where do we try to use this
info?  I see that VRP misses to use nonzero bits if no range info
is present - I suppose set_nonzero_bits misses to eventually adjust
the range.

That said, where is the folding code and why does it care for those
"uninteresting" bits at all?

> To resolve this, I added a new flag "set_by_ipa" to decl_common,
> which is set to true if the mask of parameter is determined by ipa-cp,
> and the condition changes to:
> 
> if (SSA_NAME_VAR (var)
> && TREE_CODE (SSA_NAME_VAR (var)) == PARM_DECL
> && DECL_SET_BY_IPA (SSA_NAME_VAR (var))
>   val.mask = widest_int::from (nonzero_bits,
>   TYPE_SIGN (TREE_TYPE (SSA_NAME_VAR (var)));
> else
>   val.mask = extend_mask (nonzero_bits);
> 
> I am not sure if adding a new flag to decl_common is a good idea. How
> do other ipa passes deal with this/similar issue ?
> 
> I suppose we would want to gate this on some flag, say -fipa-bit-cp ?
> I haven't yet gated it on the flag, will do in next version of patch.
> I have added some very simple test-cases, I will try to add more
> meaningful ones.

See above - we should avoid needing this.

> Patch passes bootstrap+test on x86_64-unknown-linux-gnu
> and cross-tested on arm*-*-* and aarch64*-*-* with the exception
> of some fortran tests failing due to above ICE.
> 
> As next steps, I am planning to extend it to handle alignment propagation,
> and do further testing (lto-bootstrap, chromium).
> I would be 

[RFC] ipa bitwise constant propagation

[Prathamesh Kulkarni]

Hi,
This is a prototype patch for propagating known/unknown bits inter-procedurally.
for integral types which propagates info obtained from get_nonzero_bits ().

Patch required making following changes:
a) To make info from get_nonzero_bits() available to ipa, I had to remove
guard !nonzero_p in ccp_finalize. However that triggered the following ICE
in get_ptr_info() for default_none.f95 (and several other fortran tests)
with options: -fopenacc -O2
ICE: http://pastebin.com/KjD7HMQi
I confirmed with Richard that this was a latent issue.

b) I chose widest_int for representing value, mask in ipcp_bits_lattice
and correspondingly changed declarations for
bit_value_unop_1/bit_value_binop_1 to take
precision and sign instead of type (those are the only two fields that
were used). Both these functions are exported by tree-ssa-ccp.h
I hope that's ok ?

c) Changed streamer_read_wi/streamer_write_wi to non-static.
Ah I see Kugan has submitted a patch for this, so I will drop this hunk.

d) We have following in tree-ssa-ccp.c:get_default_value ():
  if (flag_tree_bit_ccp)
{
  wide_int nonzero_bits = get_nonzero_bits (var);
  if (nonzero_bits != -1)
{
  val.lattice_val = CONSTANT;
  val.value = build_zero_cst (TREE_TYPE (var));
  val.mask = extend_mask (nonzero_bits);
}

extend_mask() sets all upper bits to 1 in nonzero_bits, ie, varying
in terms of bit-ccp.
I suppose in tree-ccp we need to extend mask if var is parameter since we don't
know in advance what values it will receive from different callers and mark all
upper bits as 1 to be safe.
However I suppose with ipa, we can determine exactly which bits of
parameter are constant and
setting all upper bits to 1 will become unnecessary ?

For example, consider following artificial test-case:
int f(int x)
{
  if (x > 300)
return 1;
  else
return 2;
}

int main(int argc, char **argv)
{
  return f(argc & 0xc) + f (argc & 0x3);
}

For x, the mask would be meet of:
<0, 0xc> meet <0, 0x3> == (0x3 | 0xc) | (0 ^ 0) == 0xf
and ipcp_update_bits() sets nonzero_bits for x to 0xf.
However get_default_value then calls extend_mask (0xf), resulting in
all upper bits
being set to 1 and consequently the condition if (x > 300) doesn't get folded.

To resolve this, I added a new flag "set_by_ipa" to decl_common,
which is set to true if the mask of parameter is determined by ipa-cp,
and the condition changes to:

if (SSA_NAME_VAR (var)
&& TREE_CODE (SSA_NAME_VAR (var)) == PARM_DECL
&& DECL_SET_BY_IPA (SSA_NAME_VAR (var))
  val.mask = widest_int::from (nonzero_bits,
  TYPE_SIGN (TREE_TYPE (SSA_NAME_VAR (var)));
else
  val.mask = extend_mask (nonzero_bits);

I am not sure if adding a new flag to decl_common is a good idea. How
do other ipa passes deal with this/similar issue ?

I suppose we would want to gate this on some flag, say -fipa-bit-cp ?
I haven't yet gated it on the flag, will do in next version of patch.
I have added some very simple test-cases, I will try to add more
meaningful ones.

Patch passes bootstrap+test on x86_64-unknown-linux-gnu
and cross-tested on arm*-*-* and aarch64*-*-* with the exception
of some fortran tests failing due to above ICE.

As next steps, I am planning to extend it to handle alignment propagation,
and do further testing (lto-bootstrap, chromium).
I would be grateful for feedback on the current patch.

Thanks,
Prathamesh
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 5b6cb9a..b770f6a 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -120,6 +120,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "params.h"
 #include "ipa-inline.h"
 #include "ipa-utils.h"
+#include "tree-ssa-ccp.h"
 
 template  class ipcp_value;
 
@@ -266,6 +267,40 @@ private:
   bool meet_with_1 (unsigned new_align, unsigned new_misalign);
 };
 
+/* Lattice of known bits, only capable of holding one value.
+   Similar to ccp_prop_value_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 lattice_val == IPA_BITS_VARYING; }
+  bool top_p () { return lattice_val == IPA_BITS_UNDEFINED; }
+  bool constant_p () { return lattice_val == IPA_BITS_CONSTANT; }
+  bool set_to_bottom ();
+  bool set_to_constant (widest_int, widest_int, signop, unsigned);
+ 
+  widest_int get_value () { return value; }
+  widest_int get_mask () { return mask; }
+  signop get_sign () { return sgn; }
+  unsigned get_precision () { return precision; }
+
+  bool meet_with (ipcp_bits_lattice& other, enum tree_code, tree);
+  bool meet_with (widest_int, widest_int, signop, unsigned);
+
+  void print (FILE *);
+
+private:
+  enum { IPA_BITS_UNDEFINED, IPA_BITS_CONSTANT, IPA_BITS_VARYING } lattice_val;
+  widest_int value, mask;
+  signop sgn;
+  unsigned precision;
+
+  bool