public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
@ 2016-11-18  1:38 kugan
  2016-11-23 10:21 ` Richard Biener
  2016-11-23 15:33 ` Martin Jambor
  0 siblings, 2 replies; 21+ messages in thread
From: kugan @ 2016-11-18  1:38 UTC (permalink / raw)
  To: Jan Hubicka, Richard Biener; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

Hi,

I was relying on ipa_get_callee_param_type to get type of parameter and 
then convert arguments to this type while computing jump functions. 
However, in cases like shown in PR78365, ipa_get_callee_param_type, 
instead of giving up, would return the wrong type. I think the current 
uses of ipa_get_callee_param_type are fine with this.

Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it 
cannot be found, then I would give up and set the jump function to varying.

Bootstrapped and regression tested on x86_64-linux-gnu with no new 
regressions. Is this OK for trunk?

Thanks,
Kugan

gcc/testsuite/ChangeLog:

2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR IPA/78365
	* gcc.dg/torture/pr78365.c: New test.

gcc/ChangeLog:

2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>

	PR IPA/78365
	* ipa-cp.c (propagate_constants_accross_call): get param type from callees
	DECL_ARGUMENTS if available.
	* ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
	(ipcp_update_vr):  Remove now redundant conversion of precision for VR.
	* ipa-prop.h: Make ipa_get_callee_param_type local again.

[-- Attachment #2: pr78365.txt --]
[-- Type: text/plain, Size: 5102 bytes --]

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2ec671f..924c846 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -2200,6 +2200,9 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
   struct ipa_edge_args *args;
   bool ret = false;
   int i, args_count, parms_count;
+  struct cgraph_node *calee = cs->callee;
+  tree fndecl = calee ? calee->decl : NULL_TREE;
+  tree parm = fndecl ? DECL_ARGUMENTS (fndecl) : NULL_TREE;
 
   callee = cs->callee->function_symbol (&availability);
   if (!callee->definition)
@@ -2247,7 +2250,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
     {
       struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
       struct ipcp_param_lattices *dest_plats;
-      tree param_type = ipa_get_callee_param_type (cs, i);
 
       dest_plats = ipa_get_parm_lattices (callee_info, i);
       if (availability == AVAIL_INTERPOSABLE)
@@ -2265,10 +2267,12 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
 	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
 	    ret |= propagate_vr_accross_jump_function (cs,
 						       jump_func, dest_plats,
-						       param_type);
+						       parm ?
+						       TREE_TYPE (parm) : NULL_TREE);
 	  else
 	    ret |= dest_plats->m_value_range.set_to_bottom ();
 	}
+      parm = parm ? DECL_CHAIN (parm) : NULL_TREE;
     }
   for (; i < parms_count; i++)
     ret |= set_all_contains_variable (ipa_get_parm_lattices (callee_info, i));
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 6321fdd..0f102c6c 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1651,7 +1651,7 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
 /* Return the Ith param type of callee associated with call graph
    edge E.  */
 
-tree
+static tree
 ipa_get_callee_param_type (struct cgraph_edge *e, int i)
 {
   int n;
@@ -1695,6 +1695,9 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
   gcall *call = cs->call_stmt;
   int n, arg_num = gimple_call_num_args (call);
   bool useful_context = false;
+  struct cgraph_node *calee = cs->callee;
+  tree fndecl = calee ? calee->decl : NULL_TREE;
+  tree parm = fndecl ? DECL_ARGUMENTS (fndecl) : NULL_TREE;
 
   if (arg_num == 0 || args->jump_functions)
     return;
@@ -1751,8 +1754,8 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	{
 	  wide_int min, max;
 	  value_range_type type;
-	  if (TREE_CODE (arg) == SSA_NAME
-	      && param_type
+	  if (parm
+	      && TREE_CODE (arg) == SSA_NAME
 	      && (type = get_range_info (arg, &min, &max))
 	      && (type == VR_RANGE || type == VR_ANTI_RANGE))
 	    {
@@ -1764,7 +1767,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	      vr.equiv = NULL;
 	      extract_range_from_unary_expr (&jfunc->m_vr,
 					     NOP_EXPR,
-					     param_type,
+					     TREE_TYPE (parm),
 					     &vr, TREE_TYPE (arg));
 	      if (jfunc->m_vr.type == VR_RANGE
 		  || jfunc->m_vr.type == VR_ANTI_RANGE)
@@ -1775,6 +1778,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	  else
 	    gcc_assert (!jfunc->vr_known);
 	}
+      parm = parm ? DECL_CHAIN (parm) : NULL_TREE;
 
       if (INTEGRAL_TYPE_P (TREE_TYPE (arg))
 	  && (TREE_CODE (arg) == SSA_NAME || TREE_CODE (arg) == INTEGER_CST))
@@ -5699,8 +5703,6 @@ ipcp_update_vr (struct cgraph_node *node)
       if (vr[i].known
 	  && (vr[i].type == VR_RANGE || vr[i].type == VR_ANTI_RANGE))
 	{
-	  tree type = TREE_TYPE (ddef);
-	  unsigned prec = TYPE_PRECISION (type);
 	  if (INTEGRAL_TYPE_P (TREE_TYPE (ddef)))
 	    {
 	      if (dump_file)
@@ -5713,11 +5715,7 @@ ipcp_update_vr (struct cgraph_node *node)
 		  print_decs (vr[i].max, dump_file);
 		  fprintf (dump_file, "]\n");
 		}
-	      set_range_info (ddef, vr[i].type,
-			      wide_int_storage::from (vr[i].min, prec,
-						      TYPE_SIGN (type)),
-			      wide_int_storage::from (vr[i].max, prec,
-						      TYPE_SIGN (type)));
+	      set_range_info (ddef, vr[i].type, vr[i].min, vr[i].max);
 	    }
 	  else if (POINTER_TYPE_P (TREE_TYPE (ddef))
 		   && vr[i].type == VR_ANTI_RANGE
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 0e75cf4..4eeae88 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -818,7 +818,6 @@ ipa_parm_adjustment *ipa_get_adjustment_candidate (tree **, bool *,
 						   ipa_parm_adjustment_vec,
 						   bool);
 void ipa_release_body_info (struct ipa_func_body_info *);
-tree ipa_get_callee_param_type (struct cgraph_edge *e, int i);
 
 /* From tree-sra.c:  */
 tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, bool, tree,
diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
index e69de29..5180a01 100644
--- a/gcc/testsuite/gcc.dg/torture/pr78365.c
+++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+int a, b, c;
+char d;
+static void fn1 (void *, int);
+int fn2 (int);
+
+void fn1 (cc, yh) void *cc;
+char yh;
+{
+  char y;
+  a = fn2(c - b + 1);
+  for (; y <= yh; y++)
+    ;
+}
+
+void fn3()
+{
+    fn1((void *)fn3, 1);
+    fn1((void *)fn3, d - 1);
+}

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-18  1:38 [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413 kugan
@ 2016-11-23 10:21 ` Richard Biener
  2016-11-23 15:33 ` Martin Jambor
  1 sibling, 0 replies; 21+ messages in thread
From: Richard Biener @ 2016-11-23 10:21 UTC (permalink / raw)
  To: kugan, Martin Jambor; +Cc: Jan Hubicka, gcc-patches

On Fri, Nov 18, 2016 at 2:38 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi,
>
> I was relying on ipa_get_callee_param_type to get type of parameter and then
> convert arguments to this type while computing jump functions. However, in
> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
> up, would return the wrong type. I think the current uses of
> ipa_get_callee_param_type are fine with this.
>
> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
> cannot be found, then I would give up and set the jump function to varying.
>
> Bootstrapped and regression tested on x86_64-linux-gnu with no new
> regressions. Is this OK for trunk?

I think a better fix for mismatches would be to record expected "source" types
in the jump functions themselves rather than trying to second-guess them.

If not then please fix ipa_get_callee_param_type -- DECL_ARGUMENTS
is indeed what we need to look at first to match the IL.  TYPE_ARG_TYPES
is merely a fallback (it doesn't have promotion applied -- see TREE_TYPE
vs. DECL_ARG_TYPE on the PARM_DECLs).

Richard.

> Thanks,
> Kugan
>
> gcc/testsuite/ChangeLog:
>
> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR IPA/78365
>         * gcc.dg/torture/pr78365.c: New test.
>
> gcc/ChangeLog:
>
> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>
>         PR IPA/78365
>         * ipa-cp.c (propagate_constants_accross_call): get param type from
> callees
>         DECL_ARGUMENTS if available.
>         * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>         (ipcp_update_vr):  Remove now redundant conversion of precision for
> VR.
>         * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-18  1:38 [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413 kugan
  2016-11-23 10:21 ` Richard Biener
@ 2016-11-23 15:33 ` Martin Jambor
  2016-11-24  8:48   ` Richard Biener
  1 sibling, 1 reply; 21+ messages in thread
From: Martin Jambor @ 2016-11-23 15:33 UTC (permalink / raw)
  To: kugan; +Cc: Jan Hubicka, Richard Biener, gcc-patches

Hi,

On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
> Hi,
> 
> I was relying on ipa_get_callee_param_type to get type of parameter and then
> convert arguments to this type while computing jump functions. However, in
> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
> up, would return the wrong type.

At what stage does this happen?  During analysis
(ipa_compute_jump_functions_for_edge) or at WPA
(propagate_constants_accross_call)?  Both?

> I think the current uses of
> ipa_get_callee_param_type are fine with this.
> 
> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
> cannot be found, then I would give up and set the jump function to varying.

But DECL_ARGUMENTS is not available at WPA stage with LTO so your
patch would make our IPA layer to optimize less with LTO.  This was
the reason to go through the hoops of TYPE_ARG_TYPES in the first
place.

If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
square one and indeed need to put the correct type in jump functions.

If just preferring DECL_ARGUMENTS is enough, then changing
ipa_get_callee_param_type to use that if is is available, as Richi
suggested, would indeed be preferable.  But if even falling back on it
can cause errors, then I am not sure if it helps.

In any event, thanks for diligently dealing with the fallout,

Martin


> 
> Bootstrapped and regression tested on x86_64-linux-gnu with no new
> regressions. Is this OK for trunk?
> 
> Thanks,
> Kugan
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR IPA/78365
> 	* gcc.dg/torture/pr78365.c: New test.
> 
> gcc/ChangeLog:
> 
> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
> 
> 	PR IPA/78365
> 	* ipa-cp.c (propagate_constants_accross_call): get param type from callees
> 	DECL_ARGUMENTS if available.
> 	* ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
> 	(ipcp_update_vr):  Remove now redundant conversion of precision for VR.
> 	* ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-23 15:33 ` Martin Jambor
@ 2016-11-24  8:48   ` Richard Biener
  2016-11-24  8:52     ` kugan
  2016-11-28  5:25     ` kugan
  0 siblings, 2 replies; 21+ messages in thread
From: Richard Biener @ 2016-11-24  8:48 UTC (permalink / raw)
  To: kugan, Jan Hubicka, Richard Biener, gcc-patches

On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
>> Hi,
>>
>> I was relying on ipa_get_callee_param_type to get type of parameter and then
>> convert arguments to this type while computing jump functions. However, in
>> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
>> up, would return the wrong type.
>
> At what stage does this happen?  During analysis
> (ipa_compute_jump_functions_for_edge) or at WPA
> (propagate_constants_accross_call)?  Both?

Hmm, where does jump function compute require the callee type?
In my view the jump function should record

 (expected-incoming-type) arg [OP X]

for each call argument in its body.  Thus required conversions are
done at WPA propagation time.

>> I think the current uses of
>> ipa_get_callee_param_type are fine with this.
>>
>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
>> cannot be found, then I would give up and set the jump function to varying.
>
> But DECL_ARGUMENTS is not available at WPA stage with LTO so your
> patch would make our IPA layer to optimize less with LTO.  This was
> the reason to go through the hoops of TYPE_ARG_TYPES in the first
> place.
>
> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
> square one and indeed need to put the correct type in jump functions.

If DECL_ARGUMENTS is not available at WPA stage then I see no other
way than to put the types on the jump functions.

> If just preferring DECL_ARGUMENTS is enough, then changing
> ipa_get_callee_param_type to use that if is is available, as Richi
> suggested, would indeed be preferable.  But if even falling back on it
> can cause errors, then I am not sure if it helps.
>
> In any event, thanks for diligently dealing with the fallout,
>
> Martin
>
>
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>> regressions. Is this OK for trunk?
>>
>> Thanks,
>> Kugan
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>       PR IPA/78365
>>       * gcc.dg/torture/pr78365.c: New test.
>>
>> gcc/ChangeLog:
>>
>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>
>>       PR IPA/78365
>>       * ipa-cp.c (propagate_constants_accross_call): get param type from callees
>>       DECL_ARGUMENTS if available.
>>       * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>>       (ipcp_update_vr):  Remove now redundant conversion of precision for VR.
>>       * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-24  8:48   ` Richard Biener
@ 2016-11-24  8:52     ` kugan
  2016-11-24  9:53       ` Jan Hubicka
  2016-11-28  5:25     ` kugan
  1 sibling, 1 reply; 21+ messages in thread
From: kugan @ 2016-11-24  8:52 UTC (permalink / raw)
  To: Richard Biener, Jan Hubicka, gcc-patches

Hi,

On 24/11/16 19:48, Richard Biener wrote:
> On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
>> Hi,
>>
>> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
>>> Hi,
>>>
>>> I was relying on ipa_get_callee_param_type to get type of parameter and thenHi,
>>> convert arguments to this type while computing jump functions. However, in
>>> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
>>> up, would return the wrong type.
>>
>> At what stage does this happen?  During analysis
>> (ipa_compute_jump_functions_for_edge) or at WPA
>> (propagate_constants_accross_call)?  Both?
>
> Hmm, where does jump function compute require the callee type?
> In my view the jump function should record
>
>  (expected-incoming-type) arg [OP X]
>
> for each call argument in its body.  Thus required conversions are
> done at WPA propagation time.
>
>>> I think the current uses of
>>> ipa_get_callee_param_type are fine with this.
>>>
>>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
>>> cannot be found, then I would give up and set the jump function to varying.
>>
>> But DECL_ARGUMENTS is not available at WPA stage with LTO so your
>> patch would make our IPA layer to optimize less with LTO.  This was
>> the reason to go through the hoops of TYPE_ARG_TYPES in the first
>> place.
>>
>> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
>> square one and indeed need to put the correct type in jump functions.
>
> If DECL_ARGUMENTS is not available at WPA stage then I see no other
> way than to put the types on the jump functions.
>
OK. I will record the type in jump function and send a revised patch.

Thanks,
Kugan

>> If just preferring DECL_ARGUMENTS is enough, then changing
>> ipa_get_callee_param_type to use that if is is available, as Richi
>> suggested, would indeed be preferable.  But if even falling back on it
>> can cause errors, then I am not sure if it helps.
>>
>> In any event, thanks for diligently dealing with the fallout,
>>
>> Martin
>>
>>
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>> regressions. Is this OK for trunk?
>>>
>>> Thanks,
>>> Kugan
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>       PR IPA/78365
>>>       * gcc.dg/torture/pr78365.c: New test.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>       PR IPA/78365
>>>       * ipa-cp.c (propagate_constants_accross_call): get param type from callees
>>>       DECL_ARGUMENTS if available.
>>>       * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>>>       (ipcp_update_vr):  Remove now redundant conversion of precision for VR.
>>>       * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-24  8:52     ` kugan
@ 2016-11-24  9:53       ` Jan Hubicka
  2016-11-24 10:15         ` Prathamesh Kulkarni
  0 siblings, 1 reply; 21+ messages in thread
From: Jan Hubicka @ 2016-11-24  9:53 UTC (permalink / raw)
  To: kugan; +Cc: Richard Biener, Jan Hubicka, gcc-patches

> >If DECL_ARGUMENTS is not available at WPA stage then I see no other
> >way than to put the types on the jump functions.
> >
> OK. I will record the type in jump function and send a revised patch.

It would be good to check how much of difference this makes to memory use
of WPA for larger program (i.e. Firefox). I guess it doesn't need to be too
bad given that most of those types are streamed for other reasons anyway.
The type lookup always seemed like a kludge, so indeed saving types to
jump functions makes sense to me.

Thanks,
Honza
> 
> Thanks,
> Kugan
> 
> >>If just preferring DECL_ARGUMENTS is enough, then changing
> >>ipa_get_callee_param_type to use that if is is available, as Richi
> >>suggested, would indeed be preferable.  But if even falling back on it
> >>can cause errors, then I am not sure if it helps.
> >>
> >>In any event, thanks for diligently dealing with the fallout,
> >>
> >>Martin
> >>
> >>
> >>>
> >>>Bootstrapped and regression tested on x86_64-linux-gnu with no new
> >>>regressions. Is this OK for trunk?
> >>>
> >>>Thanks,
> >>>Kugan
> >>>
> >>>gcc/testsuite/ChangeLog:
> >>>
> >>>2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >>>
> >>>      PR IPA/78365
> >>>      * gcc.dg/torture/pr78365.c: New test.
> >>>
> >>>gcc/ChangeLog:
> >>>
> >>>2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
> >>>
> >>>      PR IPA/78365
> >>>      * ipa-cp.c (propagate_constants_accross_call): get param type from callees
> >>>      DECL_ARGUMENTS if available.
> >>>      * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
> >>>      (ipcp_update_vr):  Remove now redundant conversion of precision for VR.
> >>>      * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-24  9:53       ` Jan Hubicka
@ 2016-11-24 10:15         ` Prathamesh Kulkarni
  2016-11-24 11:18           ` Richard Biener
  0 siblings, 1 reply; 21+ messages in thread
From: Prathamesh Kulkarni @ 2016-11-24 10:15 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: kugan, Richard Biener, gcc-patches

On 24 November 2016 at 15:23, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >If DECL_ARGUMENTS is not available at WPA stage then I see no other
>> >way than to put the types on the jump functions.
>> >
>> OK. I will record the type in jump function and send a revised patch.
>
> It would be good to check how much of difference this makes to memory use
> of WPA for larger program (i.e. Firefox). I guess it doesn't need to be too
> bad given that most of those types are streamed for other reasons anyway.
> The type lookup always seemed like a kludge, so indeed saving types to
> jump functions makes sense to me.
Similarly should the types also be saved in jump-function for ipa-bits
propagation ?

Thanks,
Prathamesh
>
> Thanks,
> Honza
>>
>> Thanks,
>> Kugan
>>
>> >>If just preferring DECL_ARGUMENTS is enough, then changing
>> >>ipa_get_callee_param_type to use that if is is available, as Richi
>> >>suggested, would indeed be preferable.  But if even falling back on it
>> >>can cause errors, then I am not sure if it helps.
>> >>
>> >>In any event, thanks for diligently dealing with the fallout,
>> >>
>> >>Martin
>> >>
>> >>
>> >>>
>> >>>Bootstrapped and regression tested on x86_64-linux-gnu with no new
>> >>>regressions. Is this OK for trunk?
>> >>>
>> >>>Thanks,
>> >>>Kugan
>> >>>
>> >>>gcc/testsuite/ChangeLog:
>> >>>
>> >>>2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>> >>>
>> >>>      PR IPA/78365
>> >>>      * gcc.dg/torture/pr78365.c: New test.
>> >>>
>> >>>gcc/ChangeLog:
>> >>>
>> >>>2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>> >>>
>> >>>      PR IPA/78365
>> >>>      * ipa-cp.c (propagate_constants_accross_call): get param type from callees
>> >>>      DECL_ARGUMENTS if available.
>> >>>      * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>> >>>      (ipcp_update_vr):  Remove now redundant conversion of precision for VR.
>> >>>      * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-24 10:15         ` Prathamesh Kulkarni
@ 2016-11-24 11:18           ` Richard Biener
  0 siblings, 0 replies; 21+ messages in thread
From: Richard Biener @ 2016-11-24 11:18 UTC (permalink / raw)
  To: Prathamesh Kulkarni; +Cc: Jan Hubicka, kugan, gcc-patches

On Thu, Nov 24, 2016 at 11:15 AM, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On 24 November 2016 at 15:23, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> >If DECL_ARGUMENTS is not available at WPA stage then I see no other
>>> >way than to put the types on the jump functions.
>>> >
>>> OK. I will record the type in jump function and send a revised patch.
>>
>> It would be good to check how much of difference this makes to memory use
>> of WPA for larger program (i.e. Firefox). I guess it doesn't need to be too
>> bad given that most of those types are streamed for other reasons anyway.
>> The type lookup always seemed like a kludge, so indeed saving types to
>> jump functions makes sense to me.
> Similarly should the types also be saved in jump-function for ipa-bits
> propagation ?

I think for all propagation (also simple constant propagation).

Richard.

> Thanks,
> Prathamesh
>>
>> Thanks,
>> Honza
>>>
>>> Thanks,
>>> Kugan
>>>
>>> >>If just preferring DECL_ARGUMENTS is enough, then changing
>>> >>ipa_get_callee_param_type to use that if is is available, as Richi
>>> >>suggested, would indeed be preferable.  But if even falling back on it
>>> >>can cause errors, then I am not sure if it helps.
>>> >>
>>> >>In any event, thanks for diligently dealing with the fallout,
>>> >>
>>> >>Martin
>>> >>
>>> >>
>>> >>>
>>> >>>Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>> >>>regressions. Is this OK for trunk?
>>> >>>
>>> >>>Thanks,
>>> >>>Kugan
>>> >>>
>>> >>>gcc/testsuite/ChangeLog:
>>> >>>
>>> >>>2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>> >>>
>>> >>>      PR IPA/78365
>>> >>>      * gcc.dg/torture/pr78365.c: New test.
>>> >>>
>>> >>>gcc/ChangeLog:
>>> >>>
>>> >>>2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>> >>>
>>> >>>      PR IPA/78365
>>> >>>      * ipa-cp.c (propagate_constants_accross_call): get param type from callees
>>> >>>      DECL_ARGUMENTS if available.
>>> >>>      * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>>> >>>      (ipcp_update_vr):  Remove now redundant conversion of precision for VR.
>>> >>>      * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-24  8:48   ` Richard Biener
  2016-11-24  8:52     ` kugan
@ 2016-11-28  5:25     ` kugan
  2016-11-28  5:40       ` Prathamesh Kulkarni
  2016-12-07 10:09       ` Martin Jambor
  1 sibling, 2 replies; 21+ messages in thread
From: kugan @ 2016-11-28  5:25 UTC (permalink / raw)
  To: Richard Biener, Jan Hubicka, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 3717 bytes --]

Hi,

On 24/11/16 19:48, Richard Biener wrote:
> On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
>> Hi,
>>
>> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
>>> Hi,
>>>
>>> I was relying on ipa_get_callee_param_type to get type of parameter and then
>>> convert arguments to this type while computing jump functions. However, in
>>> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
>>> up, would return the wrong type.
>>
>> At what stage does this happen?  During analysis
>> (ipa_compute_jump_functions_for_edge) or at WPA
>> (propagate_constants_accross_call)?  Both?
>
> Hmm, where does jump function compute require the callee type?
> In my view the jump function should record
>
>  (expected-incoming-type) arg [OP X]
>
> for each call argument in its body.  Thus required conversions are
> done at WPA propagation time.
>
>>> I think the current uses of
>>> ipa_get_callee_param_type are fine with this.
>>>
>>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
>>> cannot be found, then I would give up and set the jump function to varying.
>>
>> But DECL_ARGUMENTS is not available at WPA stage with LTO so your
>> patch would make our IPA layer to optimize less with LTO.  This was
>> the reason to go through the hoops of TYPE_ARG_TYPES in the first
>> place.
>>
>> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
>> square one and indeed need to put the correct type in jump functions.
>
> If DECL_ARGUMENTS is not available at WPA stage then I see no other
> way than to put the types on the jump functions.

Here is a patch that does this. To fox PR78365, in 
ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto 
bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux 
with no new regressions. I will build Firefox and measure the memory 
usage as Honza suggested based on the feedback.

Thanks,
Kugan



gcc/ChangeLog:

2016-11-28  Kugan Vivekanandarajah  <kugan.vivekanandarajah@linaro.org>

	PR IPA/78365
	* ipa-cp.c (propagate_vr_accross_jump_function): Remove param_type 
argument and
	use the one set in jump_func.
	(propagate_constants_accross_call): Likewise.
	* ipa-prop.c (ipa_get_callee_param_type): Chedk DECL_ARGUMENTS first.
	(ipa_compute_jump_functions_for_edge): Set param_type for jump_func.
	(ipa_write_jump_function): Stream param_type.
	(ipa_read_jump_function): Likewise.

gcc/testsuite/ChangeLog:

2016-11-28  Kugan Vivekanandarajah  <kugan.vivekanandarajah@linaro.org>

	PR IPA/78365
	* gcc.dg/torture/pr78365.c: New test.



>> If just preferring DECL_ARGUMENTS is enough, then changing
>> ipa_get_callee_param_type to use that if is is available, as Richi
>> suggested, would indeed be preferable.  But if even falling back on it
>> can cause errors, then I am not sure if it helps.
>>
>> In any event, thanks for diligently dealing with the fallout,
>>
>> Martin
>>
>>
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>> regressions. Is this OK for trunk?
>>>
>>> Thanks,
>>> Kugan
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>       PR IPA/78365
>>>       * gcc.dg/torture/pr78365.c: New test.
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>
>>>       PR IPA/78365
>>>       * ipa-cp.c (propagate_constants_accross_call): get param type from callees
>>>       DECL_ARGUMENTS if available.
>>>       * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>>>       (ipcp_update_vr):  Remove now redundant conversion of precision for VR.
>>>       * ipa-prop.h: Make ipa_get_callee_param_type local again.

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 4888 bytes --]

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2ec671f..3d50041 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1846,11 +1846,11 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
 static bool
 propagate_vr_accross_jump_function (cgraph_edge *cs,
 				    ipa_jump_func *jfunc,
-				    struct ipcp_param_lattices *dest_plats,
-				    tree param_type)
+				    struct ipcp_param_lattices *dest_plats)
 {
   struct ipcp_param_lattices *src_lats;
   ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
+  tree param_type = jfunc->param_type;
 
   if (dest_lat->bottom_p ())
     return false;
@@ -1895,9 +1895,9 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
       tree val = ipa_get_jf_constant (jfunc);
       if (TREE_CODE (val) == INTEGER_CST)
 	{
+	  val = fold_convert (param_type, val);
 	  if (TREE_OVERFLOW_P (val))
 	    val = drop_tree_overflow (val);
-	  val = fold_convert (param_type, val);
 	  jfunc->vr_known = true;
 	  jfunc->m_vr.type = VR_RANGE;
 	  jfunc->m_vr.min = val;
@@ -2247,7 +2247,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
     {
       struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
       struct ipcp_param_lattices *dest_plats;
-      tree param_type = ipa_get_callee_param_type (cs, i);
 
       dest_plats = ipa_get_parm_lattices (callee_info, i);
       if (availability == AVAIL_INTERPOSABLE)
@@ -2264,8 +2263,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
 						       dest_plats);
 	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
 	    ret |= propagate_vr_accross_jump_function (cs,
-						       jump_func, dest_plats,
-						       param_type);
+						       jump_func, dest_plats);
 	  else
 	    ret |= dest_plats->m_value_range.set_to_bottom ();
 	}
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 90c19fc..235531b 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1651,14 +1651,24 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
 /* Return the Ith param type of callee associated with call graph
    edge E.  */
 
-tree
+static tree
 ipa_get_callee_param_type (struct cgraph_edge *e, int i)
 {
   int n;
+  tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
+  if (t)
+    for (n = 0; n < i; n++)
+      {
+	if (!t)
+	  return NULL;
+	t = TREE_CHAIN (t);
+      }
+  if (t)
+    return TREE_TYPE (t);
   tree type = (e->callee
 	       ? TREE_TYPE (e->callee->decl)
 	       : gimple_call_fntype (e->call_stmt));
-  tree t = TYPE_ARG_TYPES (type);
+  t = TYPE_ARG_TYPES (type);
 
   for (n = 0; n < i; n++)
     {
@@ -1670,15 +1680,6 @@ ipa_get_callee_param_type (struct cgraph_edge *e, int i)
     return TREE_VALUE (t);
   if (!e->callee)
     return NULL;
-  t = DECL_ARGUMENTS (e->callee->decl);
-  for (n = 0; n < i; n++)
-    {
-      if (!t)
-	return NULL;
-      t = TREE_CHAIN (t);
-    }
-  if (t)
-    return TREE_TYPE (t);
   return NULL;
 }
 
@@ -1724,6 +1725,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	    useful_context = true;
 	}
 
+      jfunc->param_type = param_type;
       if (POINTER_TYPE_P (TREE_TYPE (arg)))
 	{
 	  bool addr_nonzero = false;
@@ -4709,6 +4711,7 @@ ipa_write_jump_function (struct output_block *ob,
   int i, count;
 
   streamer_write_uhwi (ob, jump_func->type);
+  stream_write_tree (ob, jump_func->param_type, true);
   switch (jump_func->type)
     {
     case IPA_JF_UNKNOWN:
@@ -4792,6 +4795,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
   int i, count;
 
   jftype = (enum jump_func_type) streamer_read_uhwi (ib);
+  jump_func->param_type = stream_read_tree (ib, data_in);
   switch (jftype)
     {
     case IPA_JF_UNKNOWN:
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 0e75cf4..8dcce99 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -180,6 +180,7 @@ struct GTY (()) ipa_jump_func
 
   /* Information about value range.  */
   bool vr_known;
+  tree  param_type;
   value_range m_vr;
 
   enum jump_func_type type;
@@ -818,7 +819,6 @@ ipa_parm_adjustment *ipa_get_adjustment_candidate (tree **, bool *,
 						   ipa_parm_adjustment_vec,
 						   bool);
 void ipa_release_body_info (struct ipa_func_body_info *);
-tree ipa_get_callee_param_type (struct cgraph_edge *e, int i);
 
 /* From tree-sra.c:  */
 tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, bool, tree,
diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
index e69de29..5180a01 100644
--- a/gcc/testsuite/gcc.dg/torture/pr78365.c
+++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+int a, b, c;
+char d;
+static void fn1 (void *, int);
+int fn2 (int);
+
+void fn1 (cc, yh) void *cc;
+char yh;
+{
+  char y;
+  a = fn2(c - b + 1);
+  for (; y <= yh; y++)
+    ;
+}
+
+void fn3()
+{
+    fn1((void *)fn3, 1);
+    fn1((void *)fn3, d - 1);
+}

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-28  5:25     ` kugan
@ 2016-11-28  5:40       ` Prathamesh Kulkarni
  2016-12-07 10:09       ` Martin Jambor
  1 sibling, 0 replies; 21+ messages in thread
From: Prathamesh Kulkarni @ 2016-11-28  5:40 UTC (permalink / raw)
  To: kugan; +Cc: Richard Biener, Jan Hubicka, gcc-patches

On 28 November 2016 at 10:55, kugan <kugan.vivekanandarajah@linaro.org> wrote:
> Hi,
>
> On 24/11/16 19:48, Richard Biener wrote:
>>
>> On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
>>>
>>> Hi,
>>>
>>> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
>>>>
>>>> Hi,
>>>>
>>>> I was relying on ipa_get_callee_param_type to get type of parameter and
>>>> then
>>>> convert arguments to this type while computing jump functions. However,
>>>> in
>>>> cases like shown in PR78365, ipa_get_callee_param_type, instead of
>>>> giving
>>>> up, would return the wrong type.
>>>
>>>
>>> At what stage does this happen?  During analysis
>>> (ipa_compute_jump_functions_for_edge) or at WPA
>>> (propagate_constants_accross_call)?  Both?
>>
>>
>> Hmm, where does jump function compute require the callee type?
>> In my view the jump function should record
>>
>>  (expected-incoming-type) arg [OP X]
>>
>> for each call argument in its body.  Thus required conversions are
>> done at WPA propagation time.
>>
>>>> I think the current uses of
>>>> ipa_get_callee_param_type are fine with this.
>>>>
>>>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
>>>> cannot be found, then I would give up and set the jump function to
>>>> varying.
>>>
>>>
>>> But DECL_ARGUMENTS is not available at WPA stage with LTO so your
>>> patch would make our IPA layer to optimize less with LTO.  This was
>>> the reason to go through the hoops of TYPE_ARG_TYPES in the first
>>> place.
>>>
>>> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
>>> square one and indeed need to put the correct type in jump functions.
>>
>>
>> If DECL_ARGUMENTS is not available at WPA stage then I see no other
>> way than to put the types on the jump functions.
>
>
> Here is a patch that does this. To fox PR78365, in
> ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto
> bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux
> with no new regressions. I will build Firefox and measure the memory usage
> as Honza suggested based on the feedback.
Hi Kugan,
In your patch in ipa_get_callee_param_type():
+ tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;

Perhaps this should be e->callee->function_symbol() ?

Thanks,
Prathamesh
>
> Thanks,
> Kugan
>
>
>
> gcc/ChangeLog:
>
> 2016-11-28  Kugan Vivekanandarajah  <kugan.vivekanandarajah@linaro.org>
>
>         PR IPA/78365
>         * ipa-cp.c (propagate_vr_accross_jump_function): Remove param_type
> argument and
>         use the one set in jump_func.
>         (propagate_constants_accross_call): Likewise.
>         * ipa-prop.c (ipa_get_callee_param_type): Chedk DECL_ARGUMENTS
> first.
>         (ipa_compute_jump_functions_for_edge): Set param_type for jump_func.
>         (ipa_write_jump_function): Stream param_type.
>         (ipa_read_jump_function): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> 2016-11-28  Kugan Vivekanandarajah  <kugan.vivekanandarajah@linaro.org>
>
>
>         PR IPA/78365
>         * gcc.dg/torture/pr78365.c: New test.
>
>
>
>>> If just preferring DECL_ARGUMENTS is enough, then changing
>>> ipa_get_callee_param_type to use that if is is available, as Richi
>>> suggested, would indeed be preferable.  But if even falling back on it
>>> can cause errors, then I am not sure if it helps.
>>>
>>> In any event, thanks for diligently dealing with the fallout,
>>>
>>> Martin
>>>
>>>
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu with no new
>>>> regressions. Is this OK for trunk?
>>>>
>>>> Thanks,
>>>> Kugan
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>>
>>>>       PR IPA/78365
>>>>       * gcc.dg/torture/pr78365.c: New test.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2016-11-18  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>>>
>>>>       PR IPA/78365
>>>>       * ipa-cp.c (propagate_constants_accross_call): get param type from
>>>> callees
>>>>       DECL_ARGUMENTS if available.
>>>>       * ipa-prop.c (ipa_compute_jump_functions_for_edge): Likewise.
>>>>       (ipcp_update_vr):  Remove now redundant conversion of precision
>>>> for VR.
>>>>       * ipa-prop.h: Make ipa_get_callee_param_type local again.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-11-28  5:25     ` kugan
  2016-11-28  5:40       ` Prathamesh Kulkarni
@ 2016-12-07 10:09       ` Martin Jambor
  2016-12-07 22:42         ` kugan
  2016-12-09  4:37         ` kugan
  1 sibling, 2 replies; 21+ messages in thread
From: Martin Jambor @ 2016-12-07 10:09 UTC (permalink / raw)
  To: kugan; +Cc: Richard Biener, Jan Hubicka, gcc-patches

Hello Kugan,

sorry, I have lost track of this patch and re-discovered it only now.

On Mon, Nov 28, 2016 at 04:25:00PM +1100, kugan wrote:
> Hi,
> 
> On 24/11/16 19:48, Richard Biener wrote:
> > On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > > Hi,
> > > 
> > > On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
> > > > Hi,
> > > > 
> > > > I was relying on ipa_get_callee_param_type to get type of parameter and then
> > > > convert arguments to this type while computing jump functions. However, in
> > > > cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
> > > > up, would return the wrong type.
> > > 
> > > At what stage does this happen?  During analysis
> > > (ipa_compute_jump_functions_for_edge) or at WPA
> > > (propagate_constants_accross_call)?  Both?
> > 
> > Hmm, where does jump function compute require the callee type?
> > In my view the jump function should record
> > 
> >  (expected-incoming-type) arg [OP X]
> > 
> > for each call argument in its body.  Thus required conversions are
> > done at WPA propagation time.
> > 
> > > > I think the current uses of
> > > > ipa_get_callee_param_type are fine with this.
> > > > 
> > > > Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
> > > > cannot be found, then I would give up and set the jump function to varying.
> > > 
> > > But DECL_ARGUMENTS is not available at WPA stage with LTO so your
> > > patch would make our IPA layer to optimize less with LTO.  This was
> > > the reason to go through the hoops of TYPE_ARG_TYPES in the first
> > > place.
> > > 
> > > If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
> > > square one and indeed need to put the correct type in jump functions.
> > 
> > If DECL_ARGUMENTS is not available at WPA stage then I see no other
> > way than to put the types on the jump functions.
> 
> Here is a patch that does this. To fox PR78365, in
> ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto
> bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux
> with no new regressions. I will build Firefox and measure the memory usage
> as Honza suggested based on the feedback.
> 

So, do you have any numbers?


> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 2ec671f..3d50041 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1846,11 +1846,11 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
>  static bool
>  propagate_vr_accross_jump_function (cgraph_edge *cs,
>  				    ipa_jump_func *jfunc,
> -				    struct ipcp_param_lattices *dest_plats,
> -				    tree param_type)
> +				    struct ipcp_param_lattices *dest_plats)
>  {
>    struct ipcp_param_lattices *src_lats;
>    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
> +  tree param_type = jfunc->param_type;
>  
>    if (dest_lat->bottom_p ())
>      return false;
> @@ -1895,9 +1895,9 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>        tree val = ipa_get_jf_constant (jfunc);
>        if (TREE_CODE (val) == INTEGER_CST)
>  	{
> +	  val = fold_convert (param_type, val);
>  	  if (TREE_OVERFLOW_P (val))
>  	    val = drop_tree_overflow (val);
> -	  val = fold_convert (param_type, val);
>  	  jfunc->vr_known = true;
>  	  jfunc->m_vr.type = VR_RANGE;
>  	  jfunc->m_vr.min = val;
> @@ -2247,7 +2247,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>      {
>        struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>        struct ipcp_param_lattices *dest_plats;
> -      tree param_type = ipa_get_callee_param_type (cs, i);
>  
>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>        if (availability == AVAIL_INTERPOSABLE)
> @@ -2264,8 +2263,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>  						       dest_plats);
>  	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
>  	    ret |= propagate_vr_accross_jump_function (cs,
> -						       jump_func, dest_plats,
> -						       param_type);
> +						       jump_func, dest_plats);
>  	  else
>  	    ret |= dest_plats->m_value_range.set_to_bottom ();
>  	}
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 90c19fc..235531b 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -1651,14 +1651,24 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
>  /* Return the Ith param type of callee associated with call graph
>     edge E.  */
>  
> -tree
> +static tree
>  ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>  {
>    int n;
> +  tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
> +  if (t)
> +    for (n = 0; n < i; n++)
> +      {
> +	if (!t)
> +	  return NULL;
> +	t = TREE_CHAIN (t);
> +      }
> +  if (t)
> +    return TREE_TYPE (t);
>    tree type = (e->callee
>  	       ? TREE_TYPE (e->callee->decl)
>  	       : gimple_call_fntype (e->call_stmt));
> -  tree t = TYPE_ARG_TYPES (type);
> +  t = TYPE_ARG_TYPES (type);

If TYPE_ARG_TYPES is inherently unreliable then I am afraid you must
not fallback on it but rather return NULL if cs->callee is not
available and adjust the caller to give up in that case.

(I have checked both testcases that we hope to fix with types in jump
functions and the gimple_call_fntype is the same as
TREE_TYPE(e->callee->decl) so that does not help either).

>  
>    for (n = 0; n < i; n++)
>      {
> @@ -1670,15 +1680,6 @@ ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>      return TREE_VALUE (t);
>    if (!e->callee)
>      return NULL;
> -  t = DECL_ARGUMENTS (e->callee->decl);
> -  for (n = 0; n < i; n++)
> -    {
> -      if (!t)
> -	return NULL;
> -      t = TREE_CHAIN (t);
> -    }
> -  if (t)
> -    return TREE_TYPE (t);
>    return NULL;
>  }
>  
> @@ -1724,6 +1725,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>  	    useful_context = true;
>  	}
>  
> +      jfunc->param_type = param_type;
>        if (POINTER_TYPE_P (TREE_TYPE (arg)))
>  	{
>  	  bool addr_nonzero = false;
> @@ -4709,6 +4711,7 @@ ipa_write_jump_function (struct output_block *ob,
>    int i, count;
>  
>    streamer_write_uhwi (ob, jump_func->type);
> +  stream_write_tree (ob, jump_func->param_type, true);
>    switch (jump_func->type)
>      {
>      case IPA_JF_UNKNOWN:
> @@ -4792,6 +4795,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
>    int i, count;
>  
>    jftype = (enum jump_func_type) streamer_read_uhwi (ib);
> +  jump_func->param_type = stream_read_tree (ib, data_in);
>    switch (jftype)
>      {
>      case IPA_JF_UNKNOWN:
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index 0e75cf4..8dcce99 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -180,6 +180,7 @@ struct GTY (()) ipa_jump_func
>  
>    /* Information about value range.  */
>    bool vr_known;
> +  tree  param_type;

Please add a comment describing what this is.

Otherwise, the intent looks fine to me.

Thanks!

Martin

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-07 10:09       ` Martin Jambor
@ 2016-12-07 22:42         ` kugan
  2016-12-09  4:37         ` kugan
  1 sibling, 0 replies; 21+ messages in thread
From: kugan @ 2016-12-07 22:42 UTC (permalink / raw)
  To: Richard Biener, Jan Hubicka, gcc-patches

Hi Martin,

On 07/12/16 21:08, Martin Jambor wrote:
> Hello Kugan,
>
> sorry, I have lost track of this patch and re-discovered it only now.
>
> On Mon, Nov 28, 2016 at 04:25:00PM +1100, kugan wrote:
>> Hi,
>>
>> On 24/11/16 19:48, Richard Biener wrote:
>>> On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
>>>> Hi,
>>>>
>>>> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
>>>>> Hi,
>>>>>
>>>>> I was relying on ipa_get_callee_param_type to get type of parameter and then
>>>>> convert arguments to this type while computing jump functions. However, in
>>>>> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
>>>>> up, would return the wrong type.
>>>>
>>>> At what stage does this happen?  During analysis
>>>> (ipa_compute_jump_functions_for_edge) or at WPA
>>>> (propagate_constants_accross_call)?  Both?
>>>
>>> Hmm, where does jump function compute require the callee type?
>>> In my view the jump function should record
>>>
>>>  (expected-incoming-type) arg [OP X]
>>>
>>> for each call argument in its body.  Thus required conversions are
>>> done at WPA propagation time.
>>>
>>>>> I think the current uses of
>>>>> ipa_get_callee_param_type are fine with this.
>>>>>
>>>>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
>>>>> cannot be found, then I would give up and set the jump function to varying.
>>>>
>>>> But DECL_ARGUMENTS is not available at WPA stage with LTO so your
>>>> patch would make our IPA layer to optimize less with LTO.  This was
>>>> the reason to go through the hoops of TYPE_ARG_TYPES in the first
>>>> place.
>>>>
>>>> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
>>>> square one and indeed need to put the correct type in jump functions.
>>>
>>> If DECL_ARGUMENTS is not available at WPA stage then I see no other
>>> way than to put the types on the jump functions.
>>
>> Here is a patch that does this. To fox PR78365, in
>> ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto
>> bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux
>> with no new regressions. I will build Firefox and measure the memory usage
>> as Honza suggested based on the feedback.
>>
>
> So, do you have any numbers?
I am starting this now. Do we have an easiest and accurate way to 
measure memory usage by gcc for Firefox compilation. Honza's LTO blog 
talks about using vmstat.


Thanks,
Kugan
>
>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index 2ec671f..3d50041 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -1846,11 +1846,11 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
>>  static bool
>>  propagate_vr_accross_jump_function (cgraph_edge *cs,
>>  				    ipa_jump_func *jfunc,
>> -				    struct ipcp_param_lattices *dest_plats,
>> -				    tree param_type)
>> +				    struct ipcp_param_lattices *dest_plats)
>>  {
>>    struct ipcp_param_lattices *src_lats;
>>    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
>> +  tree param_type = jfunc->param_type;
>>
>>    if (dest_lat->bottom_p ())
>>      return false;
>> @@ -1895,9 +1895,9 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>>        tree val = ipa_get_jf_constant (jfunc);
>>        if (TREE_CODE (val) == INTEGER_CST)
>>  	{
>> +	  val = fold_convert (param_type, val);
>>  	  if (TREE_OVERFLOW_P (val))
>>  	    val = drop_tree_overflow (val);
>> -	  val = fold_convert (param_type, val);
>>  	  jfunc->vr_known = true;
>>  	  jfunc->m_vr.type = VR_RANGE;
>>  	  jfunc->m_vr.min = val;
>> @@ -2247,7 +2247,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>>      {
>>        struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>>        struct ipcp_param_lattices *dest_plats;
>> -      tree param_type = ipa_get_callee_param_type (cs, i);
>>
>>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>>        if (availability == AVAIL_INTERPOSABLE)
>> @@ -2264,8 +2263,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>>  						       dest_plats);
>>  	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
>>  	    ret |= propagate_vr_accross_jump_function (cs,
>> -						       jump_func, dest_plats,
>> -						       param_type);
>> +						       jump_func, dest_plats);
>>  	  else
>>  	    ret |= dest_plats->m_value_range.set_to_bottom ();
>>  	}
>> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
>> index 90c19fc..235531b 100644
>> --- a/gcc/ipa-prop.c
>> +++ b/gcc/ipa-prop.c
>> @@ -1651,14 +1651,24 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
>>  /* Return the Ith param type of callee associated with call graph
>>     edge E.  */
>>
>> -tree
>> +static tree
>>  ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>>  {
>>    int n;
>> +  tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
>> +  if (t)
>> +    for (n = 0; n < i; n++)
>> +      {
>> +	if (!t)
>> +	  return NULL;
>> +	t = TREE_CHAIN (t);
>> +      }
>> +  if (t)
>> +    return TREE_TYPE (t);
>>    tree type = (e->callee
>>  	       ? TREE_TYPE (e->callee->decl)
>>  	       : gimple_call_fntype (e->call_stmt));
>> -  tree t = TYPE_ARG_TYPES (type);
>> +  t = TYPE_ARG_TYPES (type);
>
> If TYPE_ARG_TYPES is inherently unreliable then I am afraid you must
> not fallback on it but rather return NULL if cs->callee is not
> available and adjust the caller to give up in that case.
>
> (I have checked both testcases that we hope to fix with types in jump
> functions and the gimple_call_fntype is the same as
> TREE_TYPE(e->callee->decl) so that does not help either).
>
>>
>>    for (n = 0; n < i; n++)
>>      {
>> @@ -1670,15 +1680,6 @@ ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>>      return TREE_VALUE (t);
>>    if (!e->callee)
>>      return NULL;
>> -  t = DECL_ARGUMENTS (e->callee->decl);
>> -  for (n = 0; n < i; n++)
>> -    {
>> -      if (!t)
>> -	return NULL;
>> -      t = TREE_CHAIN (t);
>> -    }
>> -  if (t)
>> -    return TREE_TYPE (t);
>>    return NULL;
>>  }
>>
>> @@ -1724,6 +1725,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>>  	    useful_context = true;
>>  	}
>>
>> +      jfunc->param_type = param_type;
>>        if (POINTER_TYPE_P (TREE_TYPE (arg)))
>>  	{
>>  	  bool addr_nonzero = false;
>> @@ -4709,6 +4711,7 @@ ipa_write_jump_function (struct output_block *ob,
>>    int i, count;
>>
>>    streamer_write_uhwi (ob, jump_func->type);
>> +  stream_write_tree (ob, jump_func->param_type, true);
>>    switch (jump_func->type)
>>      {
>>      case IPA_JF_UNKNOWN:
>> @@ -4792,6 +4795,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
>>    int i, count;
>>
>>    jftype = (enum jump_func_type) streamer_read_uhwi (ib);
>> +  jump_func->param_type = stream_read_tree (ib, data_in);
>>    switch (jftype)
>>      {
>>      case IPA_JF_UNKNOWN:
>> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
>> index 0e75cf4..8dcce99 100644
>> --- a/gcc/ipa-prop.h
>> +++ b/gcc/ipa-prop.h
>> @@ -180,6 +180,7 @@ struct GTY (()) ipa_jump_func
>>
>>    /* Information about value range.  */
>>    bool vr_known;
>> +  tree  param_type;
>
> Please add a comment describing what this is.
>
> Otherwise, the intent looks fine to me.
>
> Thanks!
>
> Martin
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-07 10:09       ` Martin Jambor
  2016-12-07 22:42         ` kugan
@ 2016-12-09  4:37         ` kugan
  2016-12-09 10:51           ` Martin Jambor
  1 sibling, 1 reply; 21+ messages in thread
From: kugan @ 2016-12-09  4:37 UTC (permalink / raw)
  To: Richard Biener, Jan Hubicka, gcc-patches, Martin Jambor

[-- Attachment #1: Type: text/plain, Size: 7505 bytes --]

Hi Martin,

On 07/12/16 21:08, Martin Jambor wrote:
> Hello Kugan,
>
> sorry, I have lost track of this patch and re-discovered it only now.
>
> On Mon, Nov 28, 2016 at 04:25:00PM +1100, kugan wrote:
>> Hi,
>>
>> On 24/11/16 19:48, Richard Biener wrote:
>>> On Wed, Nov 23, 2016 at 4:33 PM, Martin Jambor <mjambor@suse.cz> wrote:
>>>> Hi,
>>>>
>>>> On Fri, Nov 18, 2016 at 12:38:18PM +1100, kugan wrote:
>>>>> Hi,
>>>>>
>>>>> I was relying on ipa_get_callee_param_type to get type of parameter and then
>>>>> convert arguments to this type while computing jump functions. However, in
>>>>> cases like shown in PR78365, ipa_get_callee_param_type, instead of giving
>>>>> up, would return the wrong type.
>>>>
>>>> At what stage does this happen?  During analysis
>>>> (ipa_compute_jump_functions_for_edge) or at WPA
>>>> (propagate_constants_accross_call)?  Both?
>>>
>>> Hmm, where does jump function compute require the callee type?
>>> In my view the jump function should record
>>>
>>>  (expected-incoming-type) arg [OP X]
>>>
>>> for each call argument in its body.  Thus required conversions are
>>> done at WPA propagation time.
>>>
>>>>> I think the current uses of
>>>>> ipa_get_callee_param_type are fine with this.
>>>>>
>>>>> Attached patch now uses callee's DECL_ARGUMENTS to get the type. If it
>>>>> cannot be found, then I would give up and set the jump function to varying.
>>>>
>>>> But DECL_ARGUMENTS is not available at WPA stage with LTO so your
>>>> patch would make our IPA layer to optimize less with LTO.  This was
>>>> the reason to go through the hoops of TYPE_ARG_TYPES in the first
>>>> place.
>>>>
>>>> If TYPE_ARG_TYPES cannot be trusted, then I'm afraid we are back to
>>>> square one and indeed need to put the correct type in jump functions.
>>>
>>> If DECL_ARGUMENTS is not available at WPA stage then I see no other
>>> way than to put the types on the jump functions.
>>
>> Here is a patch that does this. To fox PR78365, in
>> ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto
>> bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux
>> with no new regressions. I will build Firefox and measure the memory usage
>> as Honza suggested based on the feedback.
>>
>
> So, do you have any numbers?
I will do it. How do you measure the gcc's memory usage while building 
Firefox. I saw Honza's LTO blog talks about using vmstat result. Any 
tips please?

>
>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index 2ec671f..3d50041 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -1846,11 +1846,11 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
>>  static bool
>>  propagate_vr_accross_jump_function (cgraph_edge *cs,
>>  				    ipa_jump_func *jfunc,
>> -				    struct ipcp_param_lattices *dest_plats,
>> -				    tree param_type)
>> +				    struct ipcp_param_lattices *dest_plats)
>>  {
>>    struct ipcp_param_lattices *src_lats;
>>    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
>> +  tree param_type = jfunc->param_type;
>>
>>    if (dest_lat->bottom_p ())
>>      return false;
>> @@ -1895,9 +1895,9 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>>        tree val = ipa_get_jf_constant (jfunc);
>>        if (TREE_CODE (val) == INTEGER_CST)
>>  	{
>> +	  val = fold_convert (param_type, val);
>>  	  if (TREE_OVERFLOW_P (val))
>>  	    val = drop_tree_overflow (val);
>> -	  val = fold_convert (param_type, val);
>>  	  jfunc->vr_known = true;
>>  	  jfunc->m_vr.type = VR_RANGE;
>>  	  jfunc->m_vr.min = val;
>> @@ -2247,7 +2247,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>>      {
>>        struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>>        struct ipcp_param_lattices *dest_plats;
>> -      tree param_type = ipa_get_callee_param_type (cs, i);
>>
>>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>>        if (availability == AVAIL_INTERPOSABLE)
>> @@ -2264,8 +2263,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>>  						       dest_plats);
>>  	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
>>  	    ret |= propagate_vr_accross_jump_function (cs,
>> -						       jump_func, dest_plats,
>> -						       param_type);
>> +						       jump_func, dest_plats);
>>  	  else
>>  	    ret |= dest_plats->m_value_range.set_to_bottom ();
>>  	}
>> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
>> index 90c19fc..235531b 100644
>> --- a/gcc/ipa-prop.c
>> +++ b/gcc/ipa-prop.c
>> @@ -1651,14 +1651,24 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
>>  /* Return the Ith param type of callee associated with call graph
>>     edge E.  */
>>
>> -tree
>> +static tree
>>  ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>>  {
>>    int n;
>> +  tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
>> +  if (t)
>> +    for (n = 0; n < i; n++)
>> +      {
>> +	if (!t)
>> +	  return NULL;
>> +	t = TREE_CHAIN (t);
>> +      }
>> +  if (t)
>> +    return TREE_TYPE (t);
>>    tree type = (e->callee
>>  	       ? TREE_TYPE (e->callee->decl)
>>  	       : gimple_call_fntype (e->call_stmt));
>> -  tree t = TYPE_ARG_TYPES (type);
>> +  t = TYPE_ARG_TYPES (type);
>
> If TYPE_ARG_TYPES is inherently unreliable then I am afraid you must
> not fallback on it but rather return NULL if cs->callee is not
> available and adjust the caller to give up in that case.
>
> (I have checked both testcases that we hope to fix with types in jump
> functions and the gimple_call_fntype is the same as
> TREE_TYPE(e->callee->decl) so that does not help either).
Do you like the attached patch where I have removed it.


>>
>>    for (n = 0; n < i; n++)
>>      {
>> @@ -1670,15 +1680,6 @@ ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>>      return TREE_VALUE (t);
>>    if (!e->callee)
>>      return NULL;
>> -  t = DECL_ARGUMENTS (e->callee->decl);
>> -  for (n = 0; n < i; n++)
>> -    {
>> -      if (!t)
>> -	return NULL;
>> -      t = TREE_CHAIN (t);
>> -    }
>> -  if (t)
>> -    return TREE_TYPE (t);
>>    return NULL;
>>  }
>>
>> @@ -1724,6 +1725,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>>  	    useful_context = true;
>>  	}
>>
>> +      jfunc->param_type = param_type;
>>        if (POINTER_TYPE_P (TREE_TYPE (arg)))
>>  	{
>>  	  bool addr_nonzero = false;
>> @@ -4709,6 +4711,7 @@ ipa_write_jump_function (struct output_block *ob,
>>    int i, count;
>>
>>    streamer_write_uhwi (ob, jump_func->type);
>> +  stream_write_tree (ob, jump_func->param_type, true);
>>    switch (jump_func->type)
>>      {
>>      case IPA_JF_UNKNOWN:
>> @@ -4792,6 +4795,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
>>    int i, count;
>>
>>    jftype = (enum jump_func_type) streamer_read_uhwi (ib);
>> +  jump_func->param_type = stream_read_tree (ib, data_in);
>>    switch (jftype)
>>      {
>>      case IPA_JF_UNKNOWN:
>> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
>> index 0e75cf4..8dcce99 100644
>> --- a/gcc/ipa-prop.h
>> +++ b/gcc/ipa-prop.h
>> @@ -180,6 +180,7 @@ struct GTY (()) ipa_jump_func
>>
>>    /* Information about value range.  */
>>    bool vr_known;
>> +  tree  param_type;
>
> Please add a comment describing what this is.
Done. LTO bootstrapped and regression tested on x86_64-linux-gnu with 
https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00716.html and there is no 
new regressions.

Is this OK?

Thanks,
Kugan

>
> Otherwise, the intent looks fine to me.
>
> Thanks!
>
> Martin
>

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 4371 bytes --]

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2ec671f..9853467 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1846,11 +1846,11 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
 static bool
 propagate_vr_accross_jump_function (cgraph_edge *cs,
 				    ipa_jump_func *jfunc,
-				    struct ipcp_param_lattices *dest_plats,
-				    tree param_type)
+				    struct ipcp_param_lattices *dest_plats)
 {
   struct ipcp_param_lattices *src_lats;
   ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
+  tree param_type = jfunc->param_type;
 
   if (dest_lat->bottom_p ())
     return false;
@@ -2247,7 +2247,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
     {
       struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
       struct ipcp_param_lattices *dest_plats;
-      tree param_type = ipa_get_callee_param_type (cs, i);
 
       dest_plats = ipa_get_parm_lattices (callee_info, i);
       if (availability == AVAIL_INTERPOSABLE)
@@ -2264,8 +2263,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
 						       dest_plats);
 	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
 	    ret |= propagate_vr_accross_jump_function (cs,
-						       jump_func, dest_plats,
-						       param_type);
+						       jump_func, dest_plats);
 	  else
 	    ret |= dest_plats->m_value_range.set_to_bottom ();
 	}
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 642111d..21ee251 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1659,26 +1659,13 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
 /* Return the Ith param type of callee associated with call graph
    edge E.  */
 
-tree
+static tree
 ipa_get_callee_param_type (struct cgraph_edge *e, int i)
 {
   int n;
-  tree type = (e->callee
-	       ? TREE_TYPE (e->callee->decl)
-	       : gimple_call_fntype (e->call_stmt));
-  tree t = TYPE_ARG_TYPES (type);
-
-  for (n = 0; n < i; n++)
-    {
-      if (!t)
-        break;
-      t = TREE_CHAIN (t);
-    }
-  if (t)
-    return TREE_VALUE (t);
   if (!e->callee)
-    return NULL;
-  t = DECL_ARGUMENTS (e->callee->decl);
+   return NULL;
+  tree t = DECL_ARGUMENTS (e->callee->decl);
   for (n = 0; n < i; n++)
     {
       if (!t)
@@ -1732,6 +1719,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	    useful_context = true;
 	}
 
+      jfunc->param_type = param_type;
       if (POINTER_TYPE_P (TREE_TYPE (arg)))
 	{
 	  bool addr_nonzero = false;
@@ -4717,6 +4705,7 @@ ipa_write_jump_function (struct output_block *ob,
   int i, count;
 
   streamer_write_uhwi (ob, jump_func->type);
+  stream_write_tree (ob, jump_func->param_type, true);
   switch (jump_func->type)
     {
     case IPA_JF_UNKNOWN:
@@ -4800,6 +4789,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
   int i, count;
 
   jftype = (enum jump_func_type) streamer_read_uhwi (ib);
+  jump_func->param_type = stream_read_tree (ib, data_in);
   switch (jftype)
     {
     case IPA_JF_UNKNOWN:
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 0e75cf4..eeb0f6b 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -182,6 +182,9 @@ struct GTY (()) ipa_jump_func
   bool vr_known;
   value_range m_vr;
 
+  /* Type of callee param corresponding to this jump_func.  */
+  tree  param_type;
+
   enum jump_func_type type;
   /* Represents a value of a jump function.  pass_through is used only in jump
      function context.  constant represents the actual constant in constant jump
@@ -818,7 +821,6 @@ ipa_parm_adjustment *ipa_get_adjustment_candidate (tree **, bool *,
 						   ipa_parm_adjustment_vec,
 						   bool);
 void ipa_release_body_info (struct ipa_func_body_info *);
-tree ipa_get_callee_param_type (struct cgraph_edge *e, int i);
 
 /* From tree-sra.c:  */
 tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, bool, tree,
diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
index e69de29..5180a01 100644
--- a/gcc/testsuite/gcc.dg/torture/pr78365.c
+++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+int a, b, c;
+char d;
+static void fn1 (void *, int);
+int fn2 (int);
+
+void fn1 (cc, yh) void *cc;
+char yh;
+{
+  char y;
+  a = fn2(c - b + 1);
+  for (; y <= yh; y++)
+    ;
+}
+
+void fn3()
+{
+    fn1((void *)fn3, 1);
+    fn1((void *)fn3, d - 1);
+}

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-09  4:37         ` kugan
@ 2016-12-09 10:51           ` Martin Jambor
  2016-12-09 12:18             ` Richard Biener
  0 siblings, 1 reply; 21+ messages in thread
From: Martin Jambor @ 2016-12-09 10:51 UTC (permalink / raw)
  To: kugan; +Cc: Richard Biener, Jan Hubicka, gcc-patches

Hi,

On Fri, Dec 09, 2016 at 03:36:44PM +1100, kugan wrote:
> On 07/12/16 21:08, Martin Jambor wrote:
> > On Mon, Nov 28, 2016 at 04:25:00PM +1100, kugan wrote:
> >
> > ...
> >
> > > Here is a patch that does this. To fox PR78365, in
> > > ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto
> > > bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux
> > > with no new regressions. I will build Firefox and measure the memory usage
> > > as Honza suggested based on the feedback.
> > > 
> > 
> > So, do you have any numbers?
> I will do it. How do you measure the gcc's memory usage while building
> Firefox. I saw Honza's LTO blog talks about using vmstat result. Any tips
> please?

I asked Martin Liska how he does it and he replied with:

  Steps:

  1) clone git repository: https://github.com/marxin/script-misc

  2) Run command that does an LTO linking

  3) ./system_top.py > /tmp/log
  Run that in a separate terminal, terminate the script after the LTO linking
  finishes.

  4) ./vmstat_parser.py /tmp/log
  Plot the collected data.

so try that :-)   As far as I know, it uses vmstat.  Or maybe Honza
has some other method.


> > ...
> > 
> > > -tree
> > > +static tree
> > >  ipa_get_callee_param_type (struct cgraph_edge *e, int i)
> > >  {
> > >    int n;
> > > +  tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
> > > +  if (t)
> > > +    for (n = 0; n < i; n++)
> > > +      {
> > > +	if (!t)
> > > +	  return NULL;
> > > +	t = TREE_CHAIN (t);
> > > +      }
> > > +  if (t)
> > > +    return TREE_TYPE (t);
> > >    tree type = (e->callee
> > >  	       ? TREE_TYPE (e->callee->decl)
> > >  	       : gimple_call_fntype (e->call_stmt));
> > > -  tree t = TYPE_ARG_TYPES (type);
> > > +  t = TYPE_ARG_TYPES (type);
> > 
> > If TYPE_ARG_TYPES is inherently unreliable then I am afraid you must
> > not fallback on it but rather return NULL if cs->callee is not
> > available and adjust the caller to give up in that case.
> > 
> > (I have checked both testcases that we hope to fix with types in jump
> > functions and the gimple_call_fntype is the same as
> > TREE_TYPE(e->callee->decl) so that does not help either).
>
> Do you like the attached patch where I have removed it.
> 

I am fine with the new patch but you'll need an approval from Honza
or Richi.

I find it a bit saddening that we cannot really rely on
gimple_call_fntype but at least I do not see any other way.

Thanks,

Martin

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-09 10:51           ` Martin Jambor
@ 2016-12-09 12:18             ` Richard Biener
  2016-12-12  7:57               ` kugan
  2016-12-14 10:20               ` Martin Jambor
  0 siblings, 2 replies; 21+ messages in thread
From: Richard Biener @ 2016-12-09 12:18 UTC (permalink / raw)
  To: kugan, Richard Biener, Jan Hubicka, gcc-patches

On Fri, Dec 9, 2016 at 11:51 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Fri, Dec 09, 2016 at 03:36:44PM +1100, kugan wrote:
>> On 07/12/16 21:08, Martin Jambor wrote:
>> > On Mon, Nov 28, 2016 at 04:25:00PM +1100, kugan wrote:
>> >
>> > ...
>> >
>> > > Here is a patch that does this. To fox PR78365, in
>> > > ipa_get_callee_param_type, I am now checking DECL_ARGUMENTS first. I lto
>> > > bootstrapped and regression tested on x86_64-linux-gnu and ppc64le-linux
>> > > with no new regressions. I will build Firefox and measure the memory usage
>> > > as Honza suggested based on the feedback.
>> > >
>> >
>> > So, do you have any numbers?
>> I will do it. How do you measure the gcc's memory usage while building
>> Firefox. I saw Honza's LTO blog talks about using vmstat result. Any tips
>> please?
>
> I asked Martin Liska how he does it and he replied with:
>
>   Steps:
>
>   1) clone git repository: https://github.com/marxin/script-misc
>
>   2) Run command that does an LTO linking
>
>   3) ./system_top.py > /tmp/log
>   Run that in a separate terminal, terminate the script after the LTO linking
>   finishes.
>
>   4) ./vmstat_parser.py /tmp/log
>   Plot the collected data.
>
> so try that :-)   As far as I know, it uses vmstat.  Or maybe Honza
> has some other method.
>
>
>> > ...
>> >
>> > > -tree
>> > > +static tree
>> > >  ipa_get_callee_param_type (struct cgraph_edge *e, int i)
>> > >  {
>> > >    int n;
>> > > +  tree t = e->callee ? DECL_ARGUMENTS (e->callee->decl) : NULL_TREE;
>> > > +  if (t)
>> > > +    for (n = 0; n < i; n++)
>> > > +      {
>> > > + if (!t)
>> > > +   return NULL;
>> > > + t = TREE_CHAIN (t);
>> > > +      }
>> > > +  if (t)
>> > > +    return TREE_TYPE (t);
>> > >    tree type = (e->callee
>> > >          ? TREE_TYPE (e->callee->decl)
>> > >          : gimple_call_fntype (e->call_stmt));
>> > > -  tree t = TYPE_ARG_TYPES (type);
>> > > +  t = TYPE_ARG_TYPES (type);
>> >
>> > If TYPE_ARG_TYPES is inherently unreliable then I am afraid you must
>> > not fallback on it but rather return NULL if cs->callee is not
>> > available and adjust the caller to give up in that case.
>> >
>> > (I have checked both testcases that we hope to fix with types in jump
>> > functions and the gimple_call_fntype is the same as
>> > TREE_TYPE(e->callee->decl) so that does not help either).
>>
>> Do you like the attached patch where I have removed it.
>>
>
> I am fine with the new patch but you'll need an approval from Honza
> or Richi.
>
> I find it a bit saddening that we cannot really rely on
> gimple_call_fntype but at least I do not see any other way.

The patch looks somewhat backward.  It populates the param type from
the callee but the only thing we really know is the _originating_ type
from the callers DECL_ARGUMENTS (if the JF is based on a parameter
which is the only case where we do not know the type of the actual
value statically -- we only know the type we expect).

So the type would be present only on IPA_JF_PASS_THROUGH JFs
(? does that also cover modified params?).

At propagation time when applying the jump-function we have to make
sure to first convert the actual parameter value to that expected type
(or drop to BOTTOM if we can't do that conversion due to excess mismatches).

Richard.

> Thanks,
>
> Martin

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-09 12:18             ` Richard Biener
@ 2016-12-12  7:57               ` kugan
  2016-12-13 14:34                 ` Richard Biener
  2016-12-14 10:20               ` Martin Jambor
  1 sibling, 1 reply; 21+ messages in thread
From: kugan @ 2016-12-12  7:57 UTC (permalink / raw)
  To: Richard Biener, Jan Hubicka, gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

Hi Richard,

>> I am fine with the new patch but you'll need an approval from Honza
>> or Richi.
>>
>> I find it a bit saddening that we cannot really rely on
>> gimple_call_fntype but at least I do not see any other way.
>
> The patch looks somewhat backward.  It populates the param type from
> the callee but the only thing we really know is the _originating_ type
> from the callers DECL_ARGUMENTS (if the JF is based on a parameter

I am not sure I understood this. I think we get param_type from callees 
DECL_ARGUMENTS.

> which is the only case where we do not know the type of the actual
> value statically -- we only know the type we expect).
>
> So the type would be present only on IPA_JF_PASS_THROUGH JFs
> (? does that also cover modified params?).
>
> At propagation time when applying the jump-function we have to make
> sure to first convert the actual parameter value to that expected type
> (or drop to BOTTOM if we can't do that conversion due to excess mismatches).

 From ipa vrp point of view, that is what is being done with the patch 
(In this version, I also changed the POINTER_TYPE_P to use param_type). 
At the point we create ipa_compute_jump_functions_for_edge, we are 
covering the VR for arguments to param_type.

In propagate_vr_accross_jump_function also, we are doing the conversion.

Thanks,
Kugan

[-- Attachment #2: p.txt --]
[-- Type: text/plain, Size: 4833 bytes --]

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 2ec671f..9853467 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1846,11 +1846,11 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
 static bool
 propagate_vr_accross_jump_function (cgraph_edge *cs,
 				    ipa_jump_func *jfunc,
-				    struct ipcp_param_lattices *dest_plats,
-				    tree param_type)
+				    struct ipcp_param_lattices *dest_plats)
 {
   struct ipcp_param_lattices *src_lats;
   ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
+  tree param_type = jfunc->param_type;
 
   if (dest_lat->bottom_p ())
     return false;
@@ -2247,7 +2247,6 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
     {
       struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
       struct ipcp_param_lattices *dest_plats;
-      tree param_type = ipa_get_callee_param_type (cs, i);
 
       dest_plats = ipa_get_parm_lattices (callee_info, i);
       if (availability == AVAIL_INTERPOSABLE)
@@ -2264,8 +2263,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
 						       dest_plats);
 	  if (opt_for_fn (callee->decl, flag_ipa_vrp))
 	    ret |= propagate_vr_accross_jump_function (cs,
-						       jump_func, dest_plats,
-						       param_type);
+						       jump_func, dest_plats);
 	  else
 	    ret |= dest_plats->m_value_range.set_to_bottom ();
 	}
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 642111d..d9a817a 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1659,26 +1659,13 @@ determine_locally_known_aggregate_parts (gcall *call, tree arg,
 /* Return the Ith param type of callee associated with call graph
    edge E.  */
 
-tree
+static tree
 ipa_get_callee_param_type (struct cgraph_edge *e, int i)
 {
   int n;
-  tree type = (e->callee
-	       ? TREE_TYPE (e->callee->decl)
-	       : gimple_call_fntype (e->call_stmt));
-  tree t = TYPE_ARG_TYPES (type);
-
-  for (n = 0; n < i; n++)
-    {
-      if (!t)
-        break;
-      t = TREE_CHAIN (t);
-    }
-  if (t)
-    return TREE_VALUE (t);
   if (!e->callee)
-    return NULL;
-  t = DECL_ARGUMENTS (e->callee->decl);
+   return NULL;
+  tree t = DECL_ARGUMENTS (e->callee->decl);
   for (n = 0; n < i; n++)
     {
       if (!t)
@@ -1732,6 +1719,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	    useful_context = true;
 	}
 
+      jfunc->param_type = param_type;
       if (POINTER_TYPE_P (TREE_TYPE (arg)))
 	{
 	  bool addr_nonzero = false;
@@ -1748,8 +1736,8 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	    {
 	      jfunc->vr_known = true;
 	      jfunc->m_vr.type = VR_ANTI_RANGE;
-	      jfunc->m_vr.min = build_int_cst (TREE_TYPE (arg), 0);
-	      jfunc->m_vr.max = build_int_cst (TREE_TYPE (arg), 0);
+	      jfunc->m_vr.min = build_int_cst (param_type, 0);
+	      jfunc->m_vr.max = build_int_cst (param_type, 0);
 	      jfunc->m_vr.equiv = NULL;
 	    }
 	  else
@@ -4717,6 +4705,7 @@ ipa_write_jump_function (struct output_block *ob,
   int i, count;
 
   streamer_write_uhwi (ob, jump_func->type);
+  stream_write_tree (ob, jump_func->param_type, true);
   switch (jump_func->type)
     {
     case IPA_JF_UNKNOWN:
@@ -4800,6 +4789,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
   int i, count;
 
   jftype = (enum jump_func_type) streamer_read_uhwi (ib);
+  jump_func->param_type = stream_read_tree (ib, data_in);
   switch (jftype)
     {
     case IPA_JF_UNKNOWN:
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 0e75cf4..eeb0f6b 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -182,6 +182,9 @@ struct GTY (()) ipa_jump_func
   bool vr_known;
   value_range m_vr;
 
+  /* Type of callee param corresponding to this jump_func.  */
+  tree  param_type;
+
   enum jump_func_type type;
   /* Represents a value of a jump function.  pass_through is used only in jump
      function context.  constant represents the actual constant in constant jump
@@ -818,7 +821,6 @@ ipa_parm_adjustment *ipa_get_adjustment_candidate (tree **, bool *,
 						   ipa_parm_adjustment_vec,
 						   bool);
 void ipa_release_body_info (struct ipa_func_body_info *);
-tree ipa_get_callee_param_type (struct cgraph_edge *e, int i);
 
 /* From tree-sra.c:  */
 tree build_ref_for_offset (location_t, tree, HOST_WIDE_INT, bool, tree,
diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
index e69de29..5180a01 100644
--- a/gcc/testsuite/gcc.dg/torture/pr78365.c
+++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+int a, b, c;
+char d;
+static void fn1 (void *, int);
+int fn2 (int);
+
+void fn1 (cc, yh) void *cc;
+char yh;
+{
+  char y;
+  a = fn2(c - b + 1);
+  for (; y <= yh; y++)
+    ;
+}
+
+void fn3()
+{
+    fn1((void *)fn3, 1);
+    fn1((void *)fn3, d - 1);
+}

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-12  7:57               ` kugan
@ 2016-12-13 14:34                 ` Richard Biener
  0 siblings, 0 replies; 21+ messages in thread
From: Richard Biener @ 2016-12-13 14:34 UTC (permalink / raw)
  To: kugan; +Cc: Jan Hubicka, gcc-patches

On Mon, Dec 12, 2016 at 8:57 AM, kugan
<kugan.vivekanandarajah@linaro.org> wrote:
> Hi Richard,
>
>>> I am fine with the new patch but you'll need an approval from Honza
>>> or Richi.
>>>
>>> I find it a bit saddening that we cannot really rely on
>>> gimple_call_fntype but at least I do not see any other way.
>>
>>
>> The patch looks somewhat backward.  It populates the param type from
>> the callee but the only thing we really know is the _originating_ type
>> from the callers DECL_ARGUMENTS (if the JF is based on a parameter
>
>
> I am not sure I understood this. I think we get param_type from callees
> DECL_ARGUMENTS.
>
>> which is the only case where we do not know the type of the actual
>> value statically -- we only know the type we expect).
>>
>> So the type would be present only on IPA_JF_PASS_THROUGH JFs
>> (? does that also cover modified params?).
>>
>> At propagation time when applying the jump-function we have to make
>> sure to first convert the actual parameter value to that expected type
>> (or drop to BOTTOM if we can't do that conversion due to excess
>> mismatches).
>
>
> From ipa vrp point of view, that is what is being done with the patch (In
> this version, I also changed the POINTER_TYPE_P to use param_type). At the
> point we create ipa_compute_jump_functions_for_edge, we are covering the VR
> for arguments to param_type.
>
> In propagate_vr_accross_jump_function also, we are doing the conversion.

Hum, I think what your patch does is only half-way correct.  I'm
looking a bit more
into how the IPA propagation works and it looks like it's not the JF
where the type
needs to be stored but the lattice itself -- we need to know at propagation time
what type we expect for the arguments.  And if there's a mismatch with the
actual type flowing through the JF then we need to convert or punt.  And looking
closer that type should get known during propagation as at that time we should
have the actual parameter types as seen in the function body.

And ipa_get_callee_param_type should go away - just use

  jfunc->param_type = TREE_TYPE (arg);

(well, and no need to store that redundant information in the jump
function -- it's
there for all but pass-through JFs by means of the JF -- and pass-through JFs
get their type at propagation time from the source lattice values)

So we just need to get propagation correct (and not fiddle strangely with types
in JF building).

Richard.

> Thanks,
> Kugan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-09 12:18             ` Richard Biener
  2016-12-12  7:57               ` kugan
@ 2016-12-14 10:20               ` Martin Jambor
  2016-12-14 12:13                 ` Richard Biener
  1 sibling, 1 reply; 21+ messages in thread
From: Martin Jambor @ 2016-12-14 10:20 UTC (permalink / raw)
  To: Richard Biener; +Cc: kugan, Jan Hubicka, gcc-patches

Hi,

On Fri, Dec 09, 2016 at 01:18:25PM +0100, Richard Biener wrote:
> 
> The patch looks somewhat backward.  It populates the param type from
> the callee but the only thing we really know is the _originating_ type
> from the callers DECL_ARGUMENTS (if the JF is based on a parameter
> which is the only case where we do not know the type of the actual
> value statically -- we only know the type we expect).
> 
> So the type would be present only on IPA_JF_PASS_THROUGH JFs
> (? does that also cover modified params?).
> 
> At propagation time when applying the jump-function we have to make
> sure to first convert the actual parameter value to that expected type
> (or drop to BOTTOM if we can't do that conversion due to excess mismatches).
> 

I wanted to have a look at this myself for some time but the omp-low
splitting took far more time than I had anticipated and so it took my
until yesterday evening to come up with "my fix" which you can find
below.  It has the added benefit of also fixing PR 78599.

The patch removes the LTO WPA attempt to figure out types of actual
arguments from TYPE_ARG_TYPES and instead it adds streaming them into
node summaries, i.e. they come from the compile time unit with the
actual function definition (as opposed to the information we have at
call-sites when building jump functions).

Because in various corner cases and plain-garbage-code cases the types
of the actual and formal arguments can be quite different, especially
with LTO, I have reorganized propagate_vr_accross_jump_function so
that it uses extract_range_from_unary_expr in all but the
IPA_JF_PASS_THROUGH cases, because those rely on fold_convert.

In order to have all the information for
extract_range_from_unary_expr, I needed to stream also the type of the
actual argument, which hitherto we did not have for the cases when the
jump function was IPA_JF_UNKNOWN but still had vr_known set (and thus
did contain any info for constant propagation but still some info
about VR).  I am not happy about adding another field to this data
structure but at this point see not way around it.

Perhaps propagate_vr_accross_jump_function can now be simplified
further, running extract_range_from_unary_expr pretty much always (or
only when the types are not useless_type_conversion_p).  But I think I
should post what I have now.  The patch passes bootstrap, lto-boostrap
(of C, C++ and Fortran) and testing on x86_64-linux.

What do you think?

Martin

2016-12-13  Martin Jambor  <mjambor@suse.cz>

	PR ipa/78365
	* ipa-prop.h (ipa_jump_func): New field pased_type.
	* ipa-cp.c (ipa_vr_operation_and_type_effects): New function.
	(propagate_vr_accross_jump_function): Use the above function for all
	value range computations for pass-through jump functions and type
	converasion from explicit value range values.
	(ipcp_propagate_stage): Do not attempt to deduce types of formal
	parameters from TYPE_ARG_TYPES.
	* ipa-prop.c (ipa_compute_jump_functions_for_edge): Set
	jfunc->passed_type whenever setting jfunc->vr_known to true.
	(ipa_write_jump_function): Remove trailing whitespace, stream
	passed_type.
	(ipa_read_jump_function): Stream passed_type.
	(ipa_write_node_info): Stream type of the actual argument.
	(ipa_read_node_info): Likewise. Also remove trailing whitespace.

testsuite/
	* gcc.dg/torture/pr78365.c: New test.
---
 gcc/ipa-cp.c                           | 70 +++++++++++++++++-----------------
 gcc/ipa-prop.c                         | 22 ++++++++---
 gcc/ipa-prop.h                         |  3 ++
 gcc/testsuite/gcc.dg/torture/pr78365.c | 21 ++++++++++
 4 files changed, 75 insertions(+), 41 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr78365.c

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 4ec7cc5..7e6fc9a 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1839,6 +1839,23 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
     return dest_lattice->set_to_bottom ();
 }
 
+/* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
+   DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
+   the result is a range or an anti-range.  */
+
+static bool
+ipa_vr_operation_and_type_effects (value_range *dst_vr, value_range *src_vr,
+				   enum tree_code operation,
+				   tree dst_type, tree src_type)
+{
+  memset (dst_vr, 0, sizeof (*dst_vr));
+  extract_range_from_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
+  if (dst_vr->type == VR_RANGE || dst_vr->type == VR_ANTI_RANGE)
+    return true;
+  else
+    return false;
+}
+
 /* Propagate value range across jump function JFUNC that is associated with
    edge CS with param of callee of PARAM_TYPE and update DEST_PLATS
    accordingly.  */
@@ -1849,7 +1866,6 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
 				    struct ipcp_param_lattices *dest_plats,
 				    tree param_type)
 {
-  struct ipcp_param_lattices *src_lats;
   ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
 
   if (dest_lat->bottom_p ())
@@ -1862,31 +1878,23 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
 
   if (jfunc->type == IPA_JF_PASS_THROUGH)
     {
-      struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
-      int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
-      src_lats = ipa_get_parm_lattices (caller_info, src_idx);
+      enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
 
-      if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
-	return dest_lat->meet_with (src_lats->m_value_range);
-      else if (param_type
-	       && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
-		   == tcc_unary))
+      if (TREE_CODE_CLASS (operation) == tcc_unary)
 	{
-	  value_range vr;
-	  memset (&vr, 0, sizeof (vr));
+	  struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
+	  int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
 	  tree operand_type = ipa_get_type (caller_info, src_idx);
-	  enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
+	  struct ipcp_param_lattices *src_lats
+	    = ipa_get_parm_lattices (caller_info, src_idx);
 
 	  if (src_lats->m_value_range.bottom_p ())
 	    return dest_lat->set_to_bottom ();
-
-	  extract_range_from_unary_expr (&vr,
-					 operation,
-					 param_type,
-					 &src_lats->m_value_range.m_vr,
-					 operand_type);
-	  if (vr.type == VR_RANGE
-	      || vr.type == VR_ANTI_RANGE)
+	  value_range vr;
+	  if (ipa_vr_operation_and_type_effects (&vr,
+						 &src_lats->m_value_range.m_vr,
+						 operation, param_type,
+						 operand_type))
 	    return dest_lat->meet_with (&vr);
 	}
     }
@@ -1906,8 +1914,11 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
 	}
     }
 
-  if (jfunc->vr_known)
-    return dest_lat->meet_with (&jfunc->m_vr);
+  value_range vr;
+  if (jfunc->vr_known
+      && ipa_vr_operation_and_type_effects (&vr, &jfunc->m_vr, NOP_EXPR,
+					    param_type, jfunc->passed_type))
+    return dest_lat->meet_with (&vr);
   else
     return dest_lat->set_to_bottom ();
 }
@@ -2247,7 +2258,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
     {
       struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
       struct ipcp_param_lattices *dest_plats;
-      tree param_type = ipa_get_callee_param_type (cs, i);
+      tree param_type = ipa_get_type (callee_info, i);
 
       dest_plats = ipa_get_parm_lattices (callee_info, i);
       if (availability == AVAIL_INTERPOSABLE)
@@ -3234,19 +3245,6 @@ 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) && t; 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 642111d..123d422 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -1751,6 +1751,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 	      jfunc->m_vr.min = build_int_cst (TREE_TYPE (arg), 0);
 	      jfunc->m_vr.max = build_int_cst (TREE_TYPE (arg), 0);
 	      jfunc->m_vr.equiv = NULL;
+	      jfunc->passed_type = TREE_TYPE (arg);
 	    }
 	  else
 	    gcc_assert (!jfunc->vr_known);
@@ -1776,7 +1777,10 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
 					     &vr, TREE_TYPE (arg));
 	      if (jfunc->m_vr.type == VR_RANGE
 		  || jfunc->m_vr.type == VR_ANTI_RANGE)
-		jfunc->vr_known = true;
+		{
+		  jfunc->vr_known = true;
+		  jfunc->passed_type = TREE_TYPE (arg);
+		}
 	      else
 		jfunc->vr_known = false;
 	    }
@@ -4775,7 +4779,7 @@ ipa_write_jump_function (struct output_block *ob,
     {
       streamer_write_widest_int (ob, jump_func->bits.value);
       streamer_write_widest_int (ob, jump_func->bits.mask);
-    }   
+    }
   bp_pack_value (&bp, jump_func->vr_known, 1);
   streamer_write_bitpack (&bp);
   if (jump_func->vr_known)
@@ -4784,6 +4788,7 @@ ipa_write_jump_function (struct output_block *ob,
 			   VR_LAST, jump_func->m_vr.type);
       stream_write_tree (ob, jump_func->m_vr.min, true);
       stream_write_tree (ob, jump_func->m_vr.max, true);
+      stream_write_tree (ob, jump_func->passed_type, true);
     }
 }
 
@@ -4877,6 +4882,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
 						 VR_LAST);
       jump_func->m_vr.min = stream_read_tree (ib, data_in);
       jump_func->m_vr.max = stream_read_tree (ib, data_in);
+      jump_func->passed_type = stream_read_tree (ib, data_in);
     }
   else
     jump_func->vr_known = false;
@@ -4973,7 +4979,10 @@ ipa_write_node_info (struct output_block *ob, struct cgraph_node *node)
     bp_pack_value (&bp, ipa_is_param_used (info, j), 1);
   streamer_write_bitpack (&bp);
   for (j = 0; j < ipa_get_param_count (info); j++)
-    streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
+    {
+      streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
+      stream_write_tree (ob, ipa_get_type (info, j), true);
+    }
   for (e = node->callees; e; e = e->next_callee)
     {
       struct ipa_edge_args *args = IPA_EDGE_REF (e);
@@ -5020,7 +5029,7 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
 
   for (k = 0; k < ipa_get_param_count (info); k++)
     info->descriptors[k].move_cost = streamer_read_uhwi (ib);
-    
+
   bp = streamer_read_bitpack (ib);
   if (ipa_get_param_count (info) != 0)
     info->analysis_done = true;
@@ -5028,7 +5037,10 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
   for (k = 0; k < ipa_get_param_count (info); k++)
     ipa_set_param_used (info, k, bp_unpack_value (&bp, 1));
   for (k = 0; k < ipa_get_param_count (info); k++)
-    ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
+    {
+      ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
+      info->descriptors[k].decl_or_type = stream_read_tree (ib, data_in);
+    }
   for (e = node->callees; e; e = e->next_callee)
     {
       struct ipa_edge_args *args = IPA_EDGE_REF (e);
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 0e75cf4..e4de8ed 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -181,6 +181,9 @@ struct GTY (()) ipa_jump_func
   /* Information about value range.  */
   bool vr_known;
   value_range m_vr;
+  /* If value range is known, this is the type in which we pass the date at the
+     caller side.  */
+  tree passed_type;
 
   enum jump_func_type type;
   /* Represents a value of a jump function.  pass_through is used only in jump
diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
new file mode 100644
index 0000000..5180a01
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+int a, b, c;
+char d;
+static void fn1 (void *, int);
+int fn2 (int);
+
+void fn1 (cc, yh) void *cc;
+char yh;
+{
+  char y;
+  a = fn2(c - b + 1);
+  for (; y <= yh; y++)
+    ;
+}
+
+void fn3()
+{
+    fn1((void *)fn3, 1);
+    fn1((void *)fn3, d - 1);
+}
-- 
2.10.2

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-14 10:20               ` Martin Jambor
@ 2016-12-14 12:13                 ` Richard Biener
  2017-01-06 18:00                   ` Martin Jambor
  0 siblings, 1 reply; 21+ messages in thread
From: Richard Biener @ 2016-12-14 12:13 UTC (permalink / raw)
  To: Richard Biener, kugan, Jan Hubicka, gcc-patches

On Wed, Dec 14, 2016 at 11:15 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Fri, Dec 09, 2016 at 01:18:25PM +0100, Richard Biener wrote:
>>
>> The patch looks somewhat backward.  It populates the param type from
>> the callee but the only thing we really know is the _originating_ type
>> from the callers DECL_ARGUMENTS (if the JF is based on a parameter
>> which is the only case where we do not know the type of the actual
>> value statically -- we only know the type we expect).
>>
>> So the type would be present only on IPA_JF_PASS_THROUGH JFs
>> (? does that also cover modified params?).
>>
>> At propagation time when applying the jump-function we have to make
>> sure to first convert the actual parameter value to that expected type
>> (or drop to BOTTOM if we can't do that conversion due to excess mismatches).
>>
>
> I wanted to have a look at this myself for some time but the omp-low
> splitting took far more time than I had anticipated and so it took my
> until yesterday evening to come up with "my fix" which you can find
> below.  It has the added benefit of also fixing PR 78599.
>
> The patch removes the LTO WPA attempt to figure out types of actual
> arguments from TYPE_ARG_TYPES and instead it adds streaming them into
> node summaries, i.e. they come from the compile time unit with the
> actual function definition (as opposed to the information we have at
> call-sites when building jump functions).
>
> Because in various corner cases and plain-garbage-code cases the types
> of the actual and formal arguments can be quite different, especially
> with LTO, I have reorganized propagate_vr_accross_jump_function so
> that it uses extract_range_from_unary_expr in all but the
> IPA_JF_PASS_THROUGH cases, because those rely on fold_convert.
>
> In order to have all the information for
> extract_range_from_unary_expr, I needed to stream also the type of the
> actual argument, which hitherto we did not have for the cases when the
> jump function was IPA_JF_UNKNOWN but still had vr_known set (and thus
> did contain any info for constant propagation but still some info
> about VR).  I am not happy about adding another field to this data
> structure but at this point see not way around it.
>
> Perhaps propagate_vr_accross_jump_function can now be simplified
> further, running extract_range_from_unary_expr pretty much always (or
> only when the types are not useless_type_conversion_p).  But I think I
> should post what I have now.  The patch passes bootstrap, lto-boostrap
> (of C, C++ and Fortran) and testing on x86_64-linux.
>
> What do you think?
>
> Martin
>
> 2016-12-13  Martin Jambor  <mjambor@suse.cz>
>
>         PR ipa/78365
>         * ipa-prop.h (ipa_jump_func): New field pased_type.
>         * ipa-cp.c (ipa_vr_operation_and_type_effects): New function.
>         (propagate_vr_accross_jump_function): Use the above function for all
>         value range computations for pass-through jump functions and type
>         converasion from explicit value range values.
>         (ipcp_propagate_stage): Do not attempt to deduce types of formal
>         parameters from TYPE_ARG_TYPES.
>         * ipa-prop.c (ipa_compute_jump_functions_for_edge): Set
>         jfunc->passed_type whenever setting jfunc->vr_known to true.
>         (ipa_write_jump_function): Remove trailing whitespace, stream
>         passed_type.
>         (ipa_read_jump_function): Stream passed_type.
>         (ipa_write_node_info): Stream type of the actual argument.
>         (ipa_read_node_info): Likewise. Also remove trailing whitespace.
>
> testsuite/
>         * gcc.dg/torture/pr78365.c: New test.
> ---
>  gcc/ipa-cp.c                           | 70 +++++++++++++++++-----------------
>  gcc/ipa-prop.c                         | 22 ++++++++---
>  gcc/ipa-prop.h                         |  3 ++
>  gcc/testsuite/gcc.dg/torture/pr78365.c | 21 ++++++++++
>  4 files changed, 75 insertions(+), 41 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/pr78365.c
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 4ec7cc5..7e6fc9a 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1839,6 +1839,23 @@ propagate_bits_accross_jump_function (cgraph_edge *cs, int idx, ipa_jump_func *j
>      return dest_lattice->set_to_bottom ();
>  }
>
> +/* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
> +   DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
> +   the result is a range or an anti-range.  */
> +
> +static bool
> +ipa_vr_operation_and_type_effects (value_range *dst_vr, value_range *src_vr,
> +                                  enum tree_code operation,
> +                                  tree dst_type, tree src_type)
> +{
> +  memset (dst_vr, 0, sizeof (*dst_vr));

The memset is not necessary.

> +  extract_range_from_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
> +  if (dst_vr->type == VR_RANGE || dst_vr->type == VR_ANTI_RANGE)
> +    return true;
> +  else
> +    return false;
> +}
> +
>  /* Propagate value range across jump function JFUNC that is associated with
>     edge CS with param of callee of PARAM_TYPE and update DEST_PLATS
>     accordingly.  */
> @@ -1849,7 +1866,6 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>                                     struct ipcp_param_lattices *dest_plats,
>                                     tree param_type)
>  {
> -  struct ipcp_param_lattices *src_lats;
>    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
>
>    if (dest_lat->bottom_p ())
> @@ -1862,31 +1878,23 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>
>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>      {
> -      struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> -      int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
> -      src_lats = ipa_get_parm_lattices (caller_info, src_idx);
> +      enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>
> -      if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
> -       return dest_lat->meet_with (src_lats->m_value_range);
> -      else if (param_type
> -              && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> -                  == tcc_unary))
> +      if (TREE_CODE_CLASS (operation) == tcc_unary)
>         {
> -         value_range vr;
> -         memset (&vr, 0, sizeof (vr));
> +         struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> +         int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
>           tree operand_type = ipa_get_type (caller_info, src_idx);
> -         enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
> +         struct ipcp_param_lattices *src_lats
> +           = ipa_get_parm_lattices (caller_info, src_idx);
>
>           if (src_lats->m_value_range.bottom_p ())
>             return dest_lat->set_to_bottom ();
> -
> -         extract_range_from_unary_expr (&vr,
> -                                        operation,
> -                                        param_type,
> -                                        &src_lats->m_value_range.m_vr,
> -                                        operand_type);
> -         if (vr.type == VR_RANGE
> -             || vr.type == VR_ANTI_RANGE)
> +         value_range vr;
> +         if (ipa_vr_operation_and_type_effects (&vr,
> +                                                &src_lats->m_value_range.m_vr,
> +                                                operation, param_type,
> +                                                operand_type))
>             return dest_lat->meet_with (&vr);
>         }
>      }
> @@ -1906,8 +1914,11 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>         }
>      }
>
> -  if (jfunc->vr_known)
> -    return dest_lat->meet_with (&jfunc->m_vr);
> +  value_range vr;
> +  if (jfunc->vr_known
> +      && ipa_vr_operation_and_type_effects (&vr, &jfunc->m_vr, NOP_EXPR,
> +                                           param_type, jfunc->passed_type))

but instead of a new jfunc->passed_type you can use TREE_TYPE (jfunc->m_vr.min)
for example.

I notice that ipa_jump_func is badly laid out:

struct GTY (()) ipa_jump_func
{
  /* Aggregate contants description.  See struct ipa_agg_jump_function and its
     description.  */
  struct ipa_agg_jump_function agg;

  /* Information about zero/non-zero bits.  */
  struct ipa_bits bits;

  /* Information about value range.  */
  bool vr_known;
  value_range m_vr;

  enum jump_func_type type;
  /* Represents a value of a jump function.  pass_through is used only in jump
     function context.  constant represents the actual constant in constant jump
     functions and member_cst holds constant c++ member functions.  */
  union jump_func_value
  {
    struct ipa_constant_data GTY ((tag ("IPA_JF_CONST"))) constant;
    struct ipa_pass_through_data GTY ((tag ("IPA_JF_PASS_THROUGH")))
pass_through;
    struct ipa_ancestor_jf_data GTY ((tag ("IPA_JF_ANCESTOR"))) ancestor;
  } GTY ((desc ("%1.type"))) value;
};

vr_known should be moved to pack with type.  and ipa_bits::known
should be moved out
of it as well.  I also think we do not use the equiv member of m_vr
and thus that is a waste.

Not sure if memory use of this struct is an issue.

Richard.

> +    return dest_lat->meet_with (&vr);
>    else
>      return dest_lat->set_to_bottom ();
>  }
> @@ -2247,7 +2258,7 @@ propagate_constants_accross_call (struct cgraph_edge *cs)
>      {
>        struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>        struct ipcp_param_lattices *dest_plats;
> -      tree param_type = ipa_get_callee_param_type (cs, i);
> +      tree param_type = ipa_get_type (callee_info, i);
>
>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>        if (availability == AVAIL_INTERPOSABLE)
> @@ -3234,19 +3245,6 @@ 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) && t; 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 642111d..123d422 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -1751,6 +1751,7 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>               jfunc->m_vr.min = build_int_cst (TREE_TYPE (arg), 0);
>               jfunc->m_vr.max = build_int_cst (TREE_TYPE (arg), 0);
>               jfunc->m_vr.equiv = NULL;
> +             jfunc->passed_type = TREE_TYPE (arg);
>             }
>           else
>             gcc_assert (!jfunc->vr_known);
> @@ -1776,7 +1777,10 @@ ipa_compute_jump_functions_for_edge (struct ipa_func_body_info *fbi,
>                                              &vr, TREE_TYPE (arg));
>               if (jfunc->m_vr.type == VR_RANGE
>                   || jfunc->m_vr.type == VR_ANTI_RANGE)
> -               jfunc->vr_known = true;
> +               {
> +                 jfunc->vr_known = true;
> +                 jfunc->passed_type = TREE_TYPE (arg);
> +               }
>               else
>                 jfunc->vr_known = false;
>             }
> @@ -4775,7 +4779,7 @@ ipa_write_jump_function (struct output_block *ob,
>      {
>        streamer_write_widest_int (ob, jump_func->bits.value);
>        streamer_write_widest_int (ob, jump_func->bits.mask);
> -    }
> +    }
>    bp_pack_value (&bp, jump_func->vr_known, 1);
>    streamer_write_bitpack (&bp);
>    if (jump_func->vr_known)
> @@ -4784,6 +4788,7 @@ ipa_write_jump_function (struct output_block *ob,
>                            VR_LAST, jump_func->m_vr.type);
>        stream_write_tree (ob, jump_func->m_vr.min, true);
>        stream_write_tree (ob, jump_func->m_vr.max, true);
> +      stream_write_tree (ob, jump_func->passed_type, true);
>      }
>  }
>
> @@ -4877,6 +4882,7 @@ ipa_read_jump_function (struct lto_input_block *ib,
>                                                  VR_LAST);
>        jump_func->m_vr.min = stream_read_tree (ib, data_in);
>        jump_func->m_vr.max = stream_read_tree (ib, data_in);
> +      jump_func->passed_type = stream_read_tree (ib, data_in);
>      }
>    else
>      jump_func->vr_known = false;
> @@ -4973,7 +4979,10 @@ ipa_write_node_info (struct output_block *ob, struct cgraph_node *node)
>      bp_pack_value (&bp, ipa_is_param_used (info, j), 1);
>    streamer_write_bitpack (&bp);
>    for (j = 0; j < ipa_get_param_count (info); j++)
> -    streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
> +    {
> +      streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
> +      stream_write_tree (ob, ipa_get_type (info, j), true);
> +    }
>    for (e = node->callees; e; e = e->next_callee)
>      {
>        struct ipa_edge_args *args = IPA_EDGE_REF (e);
> @@ -5020,7 +5029,7 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
>
>    for (k = 0; k < ipa_get_param_count (info); k++)
>      info->descriptors[k].move_cost = streamer_read_uhwi (ib);
> -
> +
>    bp = streamer_read_bitpack (ib);
>    if (ipa_get_param_count (info) != 0)
>      info->analysis_done = true;
> @@ -5028,7 +5037,10 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
>    for (k = 0; k < ipa_get_param_count (info); k++)
>      ipa_set_param_used (info, k, bp_unpack_value (&bp, 1));
>    for (k = 0; k < ipa_get_param_count (info); k++)
> -    ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
> +    {
> +      ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
> +      info->descriptors[k].decl_or_type = stream_read_tree (ib, data_in);
> +    }
>    for (e = node->callees; e; e = e->next_callee)
>      {
>        struct ipa_edge_args *args = IPA_EDGE_REF (e);
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index 0e75cf4..e4de8ed 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -181,6 +181,9 @@ struct GTY (()) ipa_jump_func
>    /* Information about value range.  */
>    bool vr_known;
>    value_range m_vr;
> +  /* If value range is known, this is the type in which we pass the date at the
> +     caller side.  */
> +  tree passed_type;
>
>    enum jump_func_type type;
>    /* Represents a value of a jump function.  pass_through is used only in jump
> diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
> new file mode 100644
> index 0000000..5180a01
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +
> +int a, b, c;
> +char d;
> +static void fn1 (void *, int);
> +int fn2 (int);
> +
> +void fn1 (cc, yh) void *cc;
> +char yh;
> +{
> +  char y;
> +  a = fn2(c - b + 1);
> +  for (; y <= yh; y++)
> +    ;
> +}
> +
> +void fn3()
> +{
> +    fn1((void *)fn3, 1);
> +    fn1((void *)fn3, d - 1);
> +}
> --
> 2.10.2
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2016-12-14 12:13                 ` Richard Biener
@ 2017-01-06 18:00                   ` Martin Jambor
  2017-01-09 10:52                     ` Richard Biener
  0 siblings, 1 reply; 21+ messages in thread
From: Martin Jambor @ 2017-01-06 18:00 UTC (permalink / raw)
  To: Richard Biener; +Cc: kugan, Jan Hubicka, gcc-patches

Hi,

On Wed, Dec 14, 2016 at 01:12:11PM +0100, Richard Biener wrote:
> On Wed, Dec 14, 2016 at 11:15 AM, Martin Jambor <mjambor@suse.cz> wrote:

> > ...

> > +/* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
> > +   DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
> > +   the result is a range or an anti-range.  */
> > +
> > +static bool
> > +ipa_vr_operation_and_type_effects (value_range *dst_vr, value_range *src_vr,
> > +                                  enum tree_code operation,
> > +                                  tree dst_type, tree src_type)
> > +{
> > +  memset (dst_vr, 0, sizeof (*dst_vr));
> 
> The memset is not necessary.

Apparently it is.  Without it, I ended up with corrupted
dst->vr_bitmup.  I got ICEs when I removed the memset and tracked it
down to:

(gdb) p dst_vr->equiv->first->next
$14 = (bitmap_element *) 0x16

after extract_range_from_unary_expr returns.

> 
> > +  extract_range_from_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
> > +  if (dst_vr->type == VR_RANGE || dst_vr->type == VR_ANTI_RANGE)
> > +    return true;
> > +  else
> > +    return false;
> > +}
> > +
> >  /* Propagate value range across jump function JFUNC that is associated with
> >     edge CS with param of callee of PARAM_TYPE and update DEST_PLATS
> >     accordingly.  */
> > @@ -1849,7 +1866,6 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
> >                                     struct ipcp_param_lattices *dest_plats,
> >                                     tree param_type)
> >  {
> > -  struct ipcp_param_lattices *src_lats;
> >    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
> >
> >    if (dest_lat->bottom_p ())
> > @@ -1862,31 +1878,23 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
> >
> >    if (jfunc->type == IPA_JF_PASS_THROUGH)
> >      {
> > -      struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> > -      int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
> > -      src_lats = ipa_get_parm_lattices (caller_info, src_idx);
> > +      enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
> >
> > -      if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
> > -       return dest_lat->meet_with (src_lats->m_value_range);
> > -      else if (param_type
> > -              && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> > -                  == tcc_unary))
> > +      if (TREE_CODE_CLASS (operation) == tcc_unary)
> >         {
> > -         value_range vr;
> > -         memset (&vr, 0, sizeof (vr));
> > +         struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> > +         int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
> >           tree operand_type = ipa_get_type (caller_info, src_idx);
> > -         enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
> > +         struct ipcp_param_lattices *src_lats
> > +           = ipa_get_parm_lattices (caller_info, src_idx);
> >
> >           if (src_lats->m_value_range.bottom_p ())
> >             return dest_lat->set_to_bottom ();
> > -
> > -         extract_range_from_unary_expr (&vr,
> > -                                        operation,
> > -                                        param_type,
> > -                                        &src_lats->m_value_range.m_vr,
> > -                                        operand_type);
> > -         if (vr.type == VR_RANGE
> > -             || vr.type == VR_ANTI_RANGE)
> > +         value_range vr;
> > +         if (ipa_vr_operation_and_type_effects (&vr,
> > +                                                &src_lats->m_value_range.m_vr,
> > +                                                operation, param_type,
> > +                                                operand_type))
> >             return dest_lat->meet_with (&vr);
> >         }
> >      }
> > @@ -1906,8 +1914,11 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
> >         }
> >      }
> >
> > -  if (jfunc->vr_known)
> > -    return dest_lat->meet_with (&jfunc->m_vr);
> > +  value_range vr;
> > +  if (jfunc->vr_known
> > +      && ipa_vr_operation_and_type_effects (&vr, &jfunc->m_vr, NOP_EXPR,
> > +                                           param_type, jfunc->passed_type))
> 
> but instead of a new jfunc->passed_type you can use TREE_TYPE (jfunc->m_vr.min)
> for example.

Great, thanks a lot for this suggestion.  I have used that and removed
the new field addition from the patch and used your suggestion
instead.

> 
> I notice that ipa_jump_func is badly laid out:
> 
> struct GTY (()) ipa_jump_func
> {
>   /* Aggregate contants description.  See struct ipa_agg_jump_function and its
>      description.  */
>   struct ipa_agg_jump_function agg;
> 
>   /* Information about zero/non-zero bits.  */
>   struct ipa_bits bits;
> 
>   /* Information about value range.  */
>   bool vr_known;
>   value_range m_vr;
> 
>   enum jump_func_type type;
>   /* Represents a value of a jump function.  pass_through is used only in jump
>      function context.  constant represents the actual constant in constant jump
>      functions and member_cst holds constant c++ member functions.  */
>   union jump_func_value
>   {
>     struct ipa_constant_data GTY ((tag ("IPA_JF_CONST"))) constant;
>     struct ipa_pass_through_data GTY ((tag ("IPA_JF_PASS_THROUGH")))
> pass_through;
>     struct ipa_ancestor_jf_data GTY ((tag ("IPA_JF_ANCESTOR"))) ancestor;
>   } GTY ((desc ("%1.type"))) value;
> };
> 
> vr_known should be moved to pack with type.

I have swapped their position in this patch because it does not affect
anything else.

> and ipa_bits::known should be moved out of it as well.

...and will try to come up with a patch doing this soon.

> I also think we do not use the equiv member of m_vr and thus that is
> a waste.
> 
> Not sure if memory use of this struct is an issue.

We create it for each and every actual argument in every call
statement we track with IPA-CP et al, so it is visible in mem-stats of
LTO WPA of big programs.

I have bootstrapped, lto-bootstrapped and tested the following on
x86_64-linux.  OK for trunk?

Thanks,

Martin


2017-01-04  Martin Jambor  <mjambor@suse.cz>

	PR ipa/78365
	PR ipa/78599
	* ipa-prop.h (ipa_jump_func): Swap positions of vr_known and m_vr.
	* ipa-cp.c (ipa_vr_operation_and_type_effects): New function.
	(propagate_vr_accross_jump_function): Use the above function for all
	value range computations for pass-through jump functions and type
	converasion from explicit value range values.
        (ipcp_propagate_stage): Do not attempt to deduce types of formal
        parameters from TYPE_ARG_TYPES.
	* ipa-prop.c (ipa_write_jump_function): Remove trailing whitespace.
	(ipa_write_node_info): Stream type of the actual argument.
	(ipa_read_node_info): Likewise. Also remove trailing whitespace.

testsuite/
	* gcc.dg/torture/pr78365.c: New test.
---
 gcc/ipa-cp.c                           | 71 +++++++++++++++++-----------------
 gcc/ipa-prop.c                         | 14 +++++--
 gcc/ipa-prop.h                         |  5 ++-
 gcc/testsuite/gcc.dg/torture/pr78365.c | 21 ++++++++++
 4 files changed, 69 insertions(+), 42 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr78365.c

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 82bf35084b6..9cc903769e8 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1837,6 +1837,23 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
     return dest_lattice->set_to_bottom ();
 }
 
+/* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
+   DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
+   the result is a range or an anti-range.  */
+
+static bool
+ipa_vr_operation_and_type_effects (value_range *dst_vr, value_range *src_vr,
+				   enum tree_code operation,
+				   tree dst_type, tree src_type)
+{
+  memset (dst_vr, 0, sizeof (*dst_vr));
+  extract_range_from_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
+  if (dst_vr->type == VR_RANGE || dst_vr->type == VR_ANTI_RANGE)
+    return true;
+  else
+    return false;
+}
+
 /* Propagate value range across jump function JFUNC that is associated with
    edge CS with param of callee of PARAM_TYPE and update DEST_PLATS
    accordingly.  */
@@ -1846,7 +1863,6 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
 				   struct ipcp_param_lattices *dest_plats,
 				   tree param_type)
 {
-  struct ipcp_param_lattices *src_lats;
   ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
 
   if (dest_lat->bottom_p ())
@@ -1859,31 +1875,23 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
 
   if (jfunc->type == IPA_JF_PASS_THROUGH)
     {
-      struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
-      int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
-      src_lats = ipa_get_parm_lattices (caller_info, src_idx);
+      enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
 
-      if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
-	return dest_lat->meet_with (src_lats->m_value_range);
-      else if (param_type
-	       && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
-		   == tcc_unary))
+      if (TREE_CODE_CLASS (operation) == tcc_unary)
 	{
-	  value_range vr;
-	  memset (&vr, 0, sizeof (vr));
+	  struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
+	  int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
 	  tree operand_type = ipa_get_type (caller_info, src_idx);
-	  enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
+	  struct ipcp_param_lattices *src_lats
+	    = ipa_get_parm_lattices (caller_info, src_idx);
 
 	  if (src_lats->m_value_range.bottom_p ())
 	    return dest_lat->set_to_bottom ();
-
-	  extract_range_from_unary_expr (&vr,
-					 operation,
-					 param_type,
-					 &src_lats->m_value_range.m_vr,
-					 operand_type);
-	  if (vr.type == VR_RANGE
-	      || vr.type == VR_ANTI_RANGE)
+	  value_range vr;
+	  if (ipa_vr_operation_and_type_effects (&vr,
+						 &src_lats->m_value_range.m_vr,
+						 operation, param_type,
+						 operand_type))
 	    return dest_lat->meet_with (&vr);
 	}
     }
@@ -1903,8 +1911,12 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
 	}
     }
 
-  if (jfunc->vr_known)
-    return dest_lat->meet_with (&jfunc->m_vr);
+  value_range vr;
+  if (jfunc->vr_known
+      && ipa_vr_operation_and_type_effects (&vr, &jfunc->m_vr, NOP_EXPR,
+					    param_type,
+					    TREE_TYPE (jfunc->m_vr.min)))
+    return dest_lat->meet_with (&vr);
   else
     return dest_lat->set_to_bottom ();
 }
@@ -2244,7 +2256,7 @@ propagate_constants_across_call (struct cgraph_edge *cs)
     {
       struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
       struct ipcp_param_lattices *dest_plats;
-      tree param_type = ipa_get_callee_param_type (cs, i);
+      tree param_type = ipa_get_type (callee_info, i);
 
       dest_plats = ipa_get_parm_lattices (callee_info, i);
       if (availability == AVAIL_INTERPOSABLE)
@@ -3230,19 +3242,6 @@ 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) && t; 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 1afa8fc7a05..1f68f736e46 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -4775,7 +4775,7 @@ ipa_write_jump_function (struct output_block *ob,
     {
       streamer_write_widest_int (ob, jump_func->bits.value);
       streamer_write_widest_int (ob, jump_func->bits.mask);
-    }   
+    }
   bp_pack_value (&bp, jump_func->vr_known, 1);
   streamer_write_bitpack (&bp);
   if (jump_func->vr_known)
@@ -4973,7 +4973,10 @@ ipa_write_node_info (struct output_block *ob, struct cgraph_node *node)
     bp_pack_value (&bp, ipa_is_param_used (info, j), 1);
   streamer_write_bitpack (&bp);
   for (j = 0; j < ipa_get_param_count (info); j++)
-    streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
+    {
+      streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
+      stream_write_tree (ob, ipa_get_type (info, j), true);
+    }
   for (e = node->callees; e; e = e->next_callee)
     {
       struct ipa_edge_args *args = IPA_EDGE_REF (e);
@@ -5020,7 +5023,7 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
 
   for (k = 0; k < ipa_get_param_count (info); k++)
     info->descriptors[k].move_cost = streamer_read_uhwi (ib);
-    
+
   bp = streamer_read_bitpack (ib);
   if (ipa_get_param_count (info) != 0)
     info->analysis_done = true;
@@ -5028,7 +5031,10 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
   for (k = 0; k < ipa_get_param_count (info); k++)
     ipa_set_param_used (info, k, bp_unpack_value (&bp, 1));
   for (k = 0; k < ipa_get_param_count (info); k++)
-    ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
+    {
+      ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
+      info->descriptors[k].decl_or_type = stream_read_tree (ib, data_in);
+    }
   for (e = node->callees; e; e = e->next_callee)
     {
       struct ipa_edge_args *args = IPA_EDGE_REF (e);
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 341d9db6c63..c9a69ab1f53 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -178,9 +178,10 @@ struct GTY (()) ipa_jump_func
   /* Information about zero/non-zero bits.  */
   struct ipa_bits bits;
 
-  /* Information about value range.  */
-  bool vr_known;
+  /* Information about value range, containing valid data only when vr_known is
+     true.  */
   value_range m_vr;
+  bool vr_known;
 
   enum jump_func_type type;
   /* Represents a value of a jump function.  pass_through is used only in jump
diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
new file mode 100644
index 00000000000..5180a0171ae
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
@@ -0,0 +1,21 @@
+/* { dg-do compile } */
+
+int a, b, c;
+char d;
+static void fn1 (void *, int);
+int fn2 (int);
+
+void fn1 (cc, yh) void *cc;
+char yh;
+{
+  char y;
+  a = fn2(c - b + 1);
+  for (; y <= yh; y++)
+    ;
+}
+
+void fn3()
+{
+    fn1((void *)fn3, 1);
+    fn1((void *)fn3, d - 1);
+}
-- 
2.11.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
  2017-01-06 18:00                   ` Martin Jambor
@ 2017-01-09 10:52                     ` Richard Biener
  0 siblings, 0 replies; 21+ messages in thread
From: Richard Biener @ 2017-01-09 10:52 UTC (permalink / raw)
  To: Richard Biener, kugan, Jan Hubicka, gcc-patches

On Fri, Jan 6, 2017 at 7:00 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Wed, Dec 14, 2016 at 01:12:11PM +0100, Richard Biener wrote:
>> On Wed, Dec 14, 2016 at 11:15 AM, Martin Jambor <mjambor@suse.cz> wrote:
>
>> > ...
>
>> > +/* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
>> > +   DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
>> > +   the result is a range or an anti-range.  */
>> > +
>> > +static bool
>> > +ipa_vr_operation_and_type_effects (value_range *dst_vr, value_range *src_vr,
>> > +                                  enum tree_code operation,
>> > +                                  tree dst_type, tree src_type)
>> > +{
>> > +  memset (dst_vr, 0, sizeof (*dst_vr));
>>
>> The memset is not necessary.
>
> Apparently it is.  Without it, I ended up with corrupted
> dst->vr_bitmup.  I got ICEs when I removed the memset and tracked it
> down to:
>
> (gdb) p dst_vr->equiv->first->next
> $14 = (bitmap_element *) 0x16
>
> after extract_range_from_unary_expr returns.

Ah, I see that set_value_range_to_* expect properly initialized ->equiv.

>>
>> > +  extract_range_from_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
>> > +  if (dst_vr->type == VR_RANGE || dst_vr->type == VR_ANTI_RANGE)
>> > +    return true;
>> > +  else
>> > +    return false;
>> > +}
>> > +
>> >  /* Propagate value range across jump function JFUNC that is associated with
>> >     edge CS with param of callee of PARAM_TYPE and update DEST_PLATS
>> >     accordingly.  */
>> > @@ -1849,7 +1866,6 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>> >                                     struct ipcp_param_lattices *dest_plats,
>> >                                     tree param_type)
>> >  {
>> > -  struct ipcp_param_lattices *src_lats;
>> >    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
>> >
>> >    if (dest_lat->bottom_p ())
>> > @@ -1862,31 +1878,23 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>> >
>> >    if (jfunc->type == IPA_JF_PASS_THROUGH)
>> >      {
>> > -      struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
>> > -      int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
>> > -      src_lats = ipa_get_parm_lattices (caller_info, src_idx);
>> > +      enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>> >
>> > -      if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
>> > -       return dest_lat->meet_with (src_lats->m_value_range);
>> > -      else if (param_type
>> > -              && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
>> > -                  == tcc_unary))
>> > +      if (TREE_CODE_CLASS (operation) == tcc_unary)
>> >         {
>> > -         value_range vr;
>> > -         memset (&vr, 0, sizeof (vr));
>> > +         struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
>> > +         int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
>> >           tree operand_type = ipa_get_type (caller_info, src_idx);
>> > -         enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>> > +         struct ipcp_param_lattices *src_lats
>> > +           = ipa_get_parm_lattices (caller_info, src_idx);
>> >
>> >           if (src_lats->m_value_range.bottom_p ())
>> >             return dest_lat->set_to_bottom ();
>> > -
>> > -         extract_range_from_unary_expr (&vr,
>> > -                                        operation,
>> > -                                        param_type,
>> > -                                        &src_lats->m_value_range.m_vr,
>> > -                                        operand_type);
>> > -         if (vr.type == VR_RANGE
>> > -             || vr.type == VR_ANTI_RANGE)
>> > +         value_range vr;
>> > +         if (ipa_vr_operation_and_type_effects (&vr,
>> > +                                                &src_lats->m_value_range.m_vr,
>> > +                                                operation, param_type,
>> > +                                                operand_type))
>> >             return dest_lat->meet_with (&vr);
>> >         }
>> >      }
>> > @@ -1906,8 +1914,11 @@ propagate_vr_accross_jump_function (cgraph_edge *cs,
>> >         }
>> >      }
>> >
>> > -  if (jfunc->vr_known)
>> > -    return dest_lat->meet_with (&jfunc->m_vr);
>> > +  value_range vr;
>> > +  if (jfunc->vr_known
>> > +      && ipa_vr_operation_and_type_effects (&vr, &jfunc->m_vr, NOP_EXPR,
>> > +                                           param_type, jfunc->passed_type))
>>
>> but instead of a new jfunc->passed_type you can use TREE_TYPE (jfunc->m_vr.min)
>> for example.
>
> Great, thanks a lot for this suggestion.  I have used that and removed
> the new field addition from the patch and used your suggestion
> instead.
>
>>
>> I notice that ipa_jump_func is badly laid out:
>>
>> struct GTY (()) ipa_jump_func
>> {
>>   /* Aggregate contants description.  See struct ipa_agg_jump_function and its
>>      description.  */
>>   struct ipa_agg_jump_function agg;
>>
>>   /* Information about zero/non-zero bits.  */
>>   struct ipa_bits bits;
>>
>>   /* Information about value range.  */
>>   bool vr_known;
>>   value_range m_vr;
>>
>>   enum jump_func_type type;
>>   /* Represents a value of a jump function.  pass_through is used only in jump
>>      function context.  constant represents the actual constant in constant jump
>>      functions and member_cst holds constant c++ member functions.  */
>>   union jump_func_value
>>   {
>>     struct ipa_constant_data GTY ((tag ("IPA_JF_CONST"))) constant;
>>     struct ipa_pass_through_data GTY ((tag ("IPA_JF_PASS_THROUGH")))
>> pass_through;
>>     struct ipa_ancestor_jf_data GTY ((tag ("IPA_JF_ANCESTOR"))) ancestor;
>>   } GTY ((desc ("%1.type"))) value;
>> };
>>
>> vr_known should be moved to pack with type.
>
> I have swapped their position in this patch because it does not affect
> anything else.
>
>> and ipa_bits::known should be moved out of it as well.
>
> ...and will try to come up with a patch doing this soon.
>
>> I also think we do not use the equiv member of m_vr and thus that is
>> a waste.
>>
>> Not sure if memory use of this struct is an issue.
>
> We create it for each and every actual argument in every call
> statement we track with IPA-CP et al, so it is visible in mem-stats of
> LTO WPA of big programs.
>
> I have bootstrapped, lto-bootstrapped and tested the following on
> x86_64-linux.  OK for trunk?

Ok.

Thanks for fixing this.
Richard.

> Thanks,
>
> Martin
>
>
> 2017-01-04  Martin Jambor  <mjambor@suse.cz>
>
>         PR ipa/78365
>         PR ipa/78599
>         * ipa-prop.h (ipa_jump_func): Swap positions of vr_known and m_vr.
>         * ipa-cp.c (ipa_vr_operation_and_type_effects): New function.
>         (propagate_vr_accross_jump_function): Use the above function for all
>         value range computations for pass-through jump functions and type
>         converasion from explicit value range values.
>         (ipcp_propagate_stage): Do not attempt to deduce types of formal
>         parameters from TYPE_ARG_TYPES.
>         * ipa-prop.c (ipa_write_jump_function): Remove trailing whitespace.
>         (ipa_write_node_info): Stream type of the actual argument.
>         (ipa_read_node_info): Likewise. Also remove trailing whitespace.
>
> testsuite/
>         * gcc.dg/torture/pr78365.c: New test.
> ---
>  gcc/ipa-cp.c                           | 71 +++++++++++++++++-----------------
>  gcc/ipa-prop.c                         | 14 +++++--
>  gcc/ipa-prop.h                         |  5 ++-
>  gcc/testsuite/gcc.dg/torture/pr78365.c | 21 ++++++++++
>  4 files changed, 69 insertions(+), 42 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/pr78365.c
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 82bf35084b6..9cc903769e8 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1837,6 +1837,23 @@ propagate_bits_across_jump_function (cgraph_edge *cs, int idx,
>      return dest_lattice->set_to_bottom ();
>  }
>
> +/* Emulate effects of unary OPERATION and/or conversion from SRC_TYPE to
> +   DST_TYPE on value range in SRC_VR and store it to DST_VR.  Return true if
> +   the result is a range or an anti-range.  */
> +
> +static bool
> +ipa_vr_operation_and_type_effects (value_range *dst_vr, value_range *src_vr,
> +                                  enum tree_code operation,
> +                                  tree dst_type, tree src_type)
> +{
> +  memset (dst_vr, 0, sizeof (*dst_vr));
> +  extract_range_from_unary_expr (dst_vr, operation, dst_type, src_vr, src_type);
> +  if (dst_vr->type == VR_RANGE || dst_vr->type == VR_ANTI_RANGE)
> +    return true;
> +  else
> +    return false;
> +}
> +
>  /* Propagate value range across jump function JFUNC that is associated with
>     edge CS with param of callee of PARAM_TYPE and update DEST_PLATS
>     accordingly.  */
> @@ -1846,7 +1863,6 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>                                    struct ipcp_param_lattices *dest_plats,
>                                    tree param_type)
>  {
> -  struct ipcp_param_lattices *src_lats;
>    ipcp_vr_lattice *dest_lat = &dest_plats->m_value_range;
>
>    if (dest_lat->bottom_p ())
> @@ -1859,31 +1875,23 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>
>    if (jfunc->type == IPA_JF_PASS_THROUGH)
>      {
> -      struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> -      int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
> -      src_lats = ipa_get_parm_lattices (caller_info, src_idx);
> +      enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
>
> -      if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
> -       return dest_lat->meet_with (src_lats->m_value_range);
> -      else if (param_type
> -              && (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> -                  == tcc_unary))
> +      if (TREE_CODE_CLASS (operation) == tcc_unary)
>         {
> -         value_range vr;
> -         memset (&vr, 0, sizeof (vr));
> +         struct ipa_node_params *caller_info = IPA_NODE_REF (cs->caller);
> +         int src_idx = ipa_get_jf_pass_through_formal_id (jfunc);
>           tree operand_type = ipa_get_type (caller_info, src_idx);
> -         enum tree_code operation = ipa_get_jf_pass_through_operation (jfunc);
> +         struct ipcp_param_lattices *src_lats
> +           = ipa_get_parm_lattices (caller_info, src_idx);
>
>           if (src_lats->m_value_range.bottom_p ())
>             return dest_lat->set_to_bottom ();
> -
> -         extract_range_from_unary_expr (&vr,
> -                                        operation,
> -                                        param_type,
> -                                        &src_lats->m_value_range.m_vr,
> -                                        operand_type);
> -         if (vr.type == VR_RANGE
> -             || vr.type == VR_ANTI_RANGE)
> +         value_range vr;
> +         if (ipa_vr_operation_and_type_effects (&vr,
> +                                                &src_lats->m_value_range.m_vr,
> +                                                operation, param_type,
> +                                                operand_type))
>             return dest_lat->meet_with (&vr);
>         }
>      }
> @@ -1903,8 +1911,12 @@ propagate_vr_across_jump_function (cgraph_edge *cs, ipa_jump_func *jfunc,
>         }
>      }
>
> -  if (jfunc->vr_known)
> -    return dest_lat->meet_with (&jfunc->m_vr);
> +  value_range vr;
> +  if (jfunc->vr_known
> +      && ipa_vr_operation_and_type_effects (&vr, &jfunc->m_vr, NOP_EXPR,
> +                                           param_type,
> +                                           TREE_TYPE (jfunc->m_vr.min)))
> +    return dest_lat->meet_with (&vr);
>    else
>      return dest_lat->set_to_bottom ();
>  }
> @@ -2244,7 +2256,7 @@ propagate_constants_across_call (struct cgraph_edge *cs)
>      {
>        struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
>        struct ipcp_param_lattices *dest_plats;
> -      tree param_type = ipa_get_callee_param_type (cs, i);
> +      tree param_type = ipa_get_type (callee_info, i);
>
>        dest_plats = ipa_get_parm_lattices (callee_info, i);
>        if (availability == AVAIL_INTERPOSABLE)
> @@ -3230,19 +3242,6 @@ 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) && t; 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 1afa8fc7a05..1f68f736e46 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -4775,7 +4775,7 @@ ipa_write_jump_function (struct output_block *ob,
>      {
>        streamer_write_widest_int (ob, jump_func->bits.value);
>        streamer_write_widest_int (ob, jump_func->bits.mask);
> -    }
> +    }
>    bp_pack_value (&bp, jump_func->vr_known, 1);
>    streamer_write_bitpack (&bp);
>    if (jump_func->vr_known)
> @@ -4973,7 +4973,10 @@ ipa_write_node_info (struct output_block *ob, struct cgraph_node *node)
>      bp_pack_value (&bp, ipa_is_param_used (info, j), 1);
>    streamer_write_bitpack (&bp);
>    for (j = 0; j < ipa_get_param_count (info); j++)
> -    streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
> +    {
> +      streamer_write_hwi (ob, ipa_get_controlled_uses (info, j));
> +      stream_write_tree (ob, ipa_get_type (info, j), true);
> +    }
>    for (e = node->callees; e; e = e->next_callee)
>      {
>        struct ipa_edge_args *args = IPA_EDGE_REF (e);
> @@ -5020,7 +5023,7 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
>
>    for (k = 0; k < ipa_get_param_count (info); k++)
>      info->descriptors[k].move_cost = streamer_read_uhwi (ib);
> -
> +
>    bp = streamer_read_bitpack (ib);
>    if (ipa_get_param_count (info) != 0)
>      info->analysis_done = true;
> @@ -5028,7 +5031,10 @@ ipa_read_node_info (struct lto_input_block *ib, struct cgraph_node *node,
>    for (k = 0; k < ipa_get_param_count (info); k++)
>      ipa_set_param_used (info, k, bp_unpack_value (&bp, 1));
>    for (k = 0; k < ipa_get_param_count (info); k++)
> -    ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
> +    {
> +      ipa_set_controlled_uses (info, k, streamer_read_hwi (ib));
> +      info->descriptors[k].decl_or_type = stream_read_tree (ib, data_in);
> +    }
>    for (e = node->callees; e; e = e->next_callee)
>      {
>        struct ipa_edge_args *args = IPA_EDGE_REF (e);
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index 341d9db6c63..c9a69ab1f53 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -178,9 +178,10 @@ struct GTY (()) ipa_jump_func
>    /* Information about zero/non-zero bits.  */
>    struct ipa_bits bits;
>
> -  /* Information about value range.  */
> -  bool vr_known;
> +  /* Information about value range, containing valid data only when vr_known is
> +     true.  */
>    value_range m_vr;
> +  bool vr_known;
>
>    enum jump_func_type type;
>    /* Represents a value of a jump function.  pass_through is used only in jump
> diff --git a/gcc/testsuite/gcc.dg/torture/pr78365.c b/gcc/testsuite/gcc.dg/torture/pr78365.c
> new file mode 100644
> index 00000000000..5180a0171ae
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr78365.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +
> +int a, b, c;
> +char d;
> +static void fn1 (void *, int);
> +int fn2 (int);
> +
> +void fn1 (cc, yh) void *cc;
> +char yh;
> +{
> +  char y;
> +  a = fn2(c - b + 1);
> +  for (; y <= yh; y++)
> +    ;
> +}
> +
> +void fn3()
> +{
> +    fn1((void *)fn3, 1);
> +    fn1((void *)fn3, d - 1);
> +}
> --
> 2.11.0
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2017-01-09 10:52 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-18  1:38 [PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413 kugan
2016-11-23 10:21 ` Richard Biener
2016-11-23 15:33 ` Martin Jambor
2016-11-24  8:48   ` Richard Biener
2016-11-24  8:52     ` kugan
2016-11-24  9:53       ` Jan Hubicka
2016-11-24 10:15         ` Prathamesh Kulkarni
2016-11-24 11:18           ` Richard Biener
2016-11-28  5:25     ` kugan
2016-11-28  5:40       ` Prathamesh Kulkarni
2016-12-07 10:09       ` Martin Jambor
2016-12-07 22:42         ` kugan
2016-12-09  4:37         ` kugan
2016-12-09 10:51           ` Martin Jambor
2016-12-09 12:18             ` Richard Biener
2016-12-12  7:57               ` kugan
2016-12-13 14:34                 ` Richard Biener
2016-12-14 10:20               ` Martin Jambor
2016-12-14 12:13                 ` Richard Biener
2017-01-06 18:00                   ` Martin Jambor
2017-01-09 10:52                     ` Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).