[PR78365] ICE in determine_value_range, at tree-ssa-loo p-niter.c:413
kugan
kugan.vivekanandarajah@linaro.org
Fri Dec 9 04:37:00 GMT 2016
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
>
-------------- next part --------------
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);
+}
More information about the Gcc-patches
mailing list