Confirmed, we really wrongly propagate 0 to x:

$ cat xxx.ltrans0.075i.cp
;; Function foo.constprop (foo.constprop.1, funcdef_no=3, decl_uid=4078, cgraph_uid=1, symbol_order=5) (executed once)

Modification phase of node foo.constprop/5
Adjusting mask for param 0 to 0x0
Setting nonnull for 0
foo.constprop (double * a)
{
  double x;

  <bb 3> [100.00%] [count: INV]:

  <bb 2> [100.00%] [count: INV]:
  *a_2(D) = x_1(D);
  return;

}

Started with r241990.

Works fine with -O2 -fno-inline, but aborts with -flto -O2 -fno-inline.
Does this possibly indicate an issue with streaming ?

Thanks,
Prathamesh

(In reply to prathamesh3492 from comment #2)
> Works fine with -O2 -fno-inline, but aborts with -flto -O2 -fno-inline.
> Does this possibly indicate an issue with streaming ?
Oops, no. Changing the definitions to static makes the test-case abort with -O2. Sorry for the noise.
> 
> Thanks,
> Prathamesh

Created attachment 42535 [details]
Untested fix

Hi,
The issue here for propagating value of 'm' from f_c1 to foo() is that the jump function operation is FLOAT_EXPR, and the type of input param 'm' is int, so fold_unary() doesn't do the conversion to real_type. The attached patch fixes that by calling fold_convert if operation is FLOAT_EXPR and converts it to the type of corresponding parameter in callee.
Does this look in the right direction ?
I will validate the patch and post upstream if there are no regressions.

Thanks,
Prathamesh

(In reply to prathamesh3492 from comment #4)
> Created attachment 42535 [details]
> Untested fix
> 

> Hi,
> The issue here for propagating value of 'm' from f_c1 to foo() is that the
> jump function operation is FLOAT_EXPR, and the type of input param 'm' is
> int, so fold_unary() doesn't do the conversion to real_type.

Thanks a lot for the quick the analysis.

> The attached
> patch fixes that by calling fold_convert if operation is FLOAT_EXPR and
> converts it to the type of corresponding parameter in callee.
> Does this look in the right direction ?

> --- a/gcc/ipa-cp.c	
> +++ a/gcc/ipa-cp.c	
> @@ -1233,7 +1234,13 @@ ipa_get_jf_pass_through_result (struct
> ipa_jump_func *j> func, tree input)
>    if (!is_gimple_ip_invariant (input))
>      return NULL_TREE;
>  
> -  if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> +  if (parm_type &&
> +      ipa_get_jf_pass_through_operation (jfunc) == FLOAT_EXPR)
> +    {
> +      res = fold_convert (parm_type, input);
> +      restype = parm_type;
> +    }
> +  else if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))

I don't think it is correct to make parm_type a parameter with NULL
default value and then only handle FLOAT_EXPR safely when it arrives
through paths that actually provide a non-NULL value.

So either we should just always return NULL for FLOAT_EXPR operation
or return NULL_TREE when operation is FLOAT_EXPR and param_tree is
NULL (or provide the type from all callers but I can see that might be
a bit painful).

DO I assume correctly that we have the same problem with
FIX_TRUNC_EXPR too?

(In reply to Martin Jambor from comment #5)
> (In reply to prathamesh3492 from comment #4)
> > Created attachment 42535 [details]
> > Untested fix
> > 
> 
> > Hi,
> > The issue here for propagating value of 'm' from f_c1 to foo() is that the
> > jump function operation is FLOAT_EXPR, and the type of input param 'm' is
> > int, so fold_unary() doesn't do the conversion to real_type.
> 
> Thanks a lot for the quick the analysis.
> 
> > The attached
> > patch fixes that by calling fold_convert if operation is FLOAT_EXPR and
> > converts it to the type of corresponding parameter in callee.
> > Does this look in the right direction ?
> 
> > --- a/gcc/ipa-cp.c	
> > +++ a/gcc/ipa-cp.c	
> > @@ -1233,7 +1234,13 @@ ipa_get_jf_pass_through_result (struct
> > ipa_jump_func *j> func, tree input)
> >    if (!is_gimple_ip_invariant (input))
> >      return NULL_TREE;
> >  
> > -  if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> > +  if (parm_type &&
> > +      ipa_get_jf_pass_through_operation (jfunc) == FLOAT_EXPR)
> > +    {
> > +      res = fold_convert (parm_type, input);
> > +      restype = parm_type;
> > +    }
> > +  else if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> 
> I don't think it is correct to make parm_type a parameter with NULL
> default value and then only handle FLOAT_EXPR safely when it arrives
> through paths that actually provide a non-NULL value.
> 
> So either we should just always return NULL for FLOAT_EXPR operation
> or return NULL_TREE when operation is FLOAT_EXPR and param_tree is
> NULL (or provide the type from all callers but I can see that might be
> a bit painful).
Indeed, I will modify the patch to return NULL_TREE when operation is FLOAT_EXPR and parm_type is NULL.
> 
> DO I assume correctly that we have the same problem with
> FIX_TRUNC_EXPR too?
I suppose yes, I will add case for FIX_TRUNC_EXPR in the patch. I am not sure though how to write a C test-case for generating FIX_TRUNC_EXPR ?

Thanks,
Prathamesh

ipa_get_jf_pass_through_result has multiple issues:

static tree
ipa_get_jf_pass_through_result (struct ipa_jump_func *jfunc, tree input)
{
  tree restype, res;

  if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
    return input;

we may miss a truncation here (and should check for CONVERT_EXPR_CODE_P).

  if (!is_gimple_ip_invariant (input))
    return NULL_TREE;

  if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
      == tcc_unary)
    res = fold_unary (ipa_get_jf_pass_through_operation (jfunc),
                      TREE_TYPE (input), input);

the bug here is that the operation is type-changing but we use the
type of the input(!) rather than the type of the output when computing
the result.  I suppose the output is the type of the current formal
parameter -- is that available somehow?

  else
    {
      if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
          == tcc_comparison)
        restype = boolean_type_node;
      else
        restype = TREE_TYPE (input);

same issue for operations like widen_sum_expr but of course less likely
to hit us here.

      res = fold_binary (ipa_get_jf_pass_through_operation (jfunc), restype,
                         input, ipa_get_jf_pass_through_operand (jfunc));
    }
  if (res && !is_gimple_ip_invariant (res))
    return NULL_TREE;


I think the immediate thing to do is make the list of handled expression
codes explicit - thus change the tree-code-class checks to

switch (code)
 {
 case handled-unary:
  fold_unary...

 case handled-comparison:
  ...

 etc.

or somehow properly get the jf "target" type.

(In reply to Richard Biener from comment #7)
> ipa_get_jf_pass_through_result has multiple issues:
> 
> static tree
> ipa_get_jf_pass_through_result (struct ipa_jump_func *jfunc, tree input)
> {
>   tree restype, res;
> 
>   if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
>     return input;
> 
> we may miss a truncation here (and should check for CONVERT_EXPR_CODE_P).

I admit this is confusing but no.  Until gcc7 we had two kinds of
pass-through jump functions, binary and really_no_operation_at_all.
NOP_EXPR was used to mark that more simple case and still is.
However, despite having unary pass-through jump functions now, we
never construct those with if the operation would be a
CONVERT_EXPR_CODE_P.  Maybe we should use ERROR_MARK or something
instead of NOP_EXPR.

> 
>   if (!is_gimple_ip_invariant (input))
>     return NULL_TREE;
> 
>   if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
>       == tcc_unary)
>     res = fold_unary (ipa_get_jf_pass_through_operation (jfunc),
>                       TREE_TYPE (input), input);
> 
> the bug here is that the operation is type-changing but we use the
> type of the input(!) rather than the type of the output when computing
> the result.  I suppose the output is the type of the current formal
> parameter -- is that available somehow?

The output in the original IL was the actual argument, at IPA level we
generally assume that it is compatible with the callee formal
argument.  That type is now available from the new parameter that
Prathamesh is adding with his patch, but he does not pass them from
all callers.  If the type is required for more than a bunch of
not-likely-important operations, we should try harder and provide it
from everywhere (possibly incrementally).

> 
>   else
>     {
>       if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
>           == tcc_comparison)
>         restype = boolean_type_node;
>       else
>         restype = TREE_TYPE (input);
> 
> same issue for operations like widen_sum_expr but of course less likely
> to hit us here.
> 
>       res = fold_binary (ipa_get_jf_pass_through_operation (jfunc), restype,
>                          input, ipa_get_jf_pass_through_operand (jfunc));
>     }
>   if (res && !is_gimple_ip_invariant (res))
>     return NULL_TREE;
> 
> 
> I think the immediate thing to do is make the list of handled expression
> codes explicit - thus change the tree-code-class checks to
> 
> switch (code)
>  {
>  case handled-unary:
>   fold_unary...
> 
>  case handled-comparison:
>   ...
> 
>  etc.
> 
> or somehow properly get the jf "target" type.

Nowadays it should be possible to do the latter (but people always
frown when I introduce more type handling to WPA, so I would not
necessarily object to the former either).

Author: jamborm
Date: Tue Nov 28 18:52:49 2017
New Revision: 255212

URL: https://gcc.gnu.org/viewcvs?rev=255212&root=gcc&view=rev
Log:
[PR 82808] Use proper result types for arithmetic jump functions

2017-11-28  Prathamesh Kulkarni  <prathamesh.kulkarni@linaro.org>
	    Martin Jambor  <mjambor@suse.cz>

	PR ipa/82808
	* tree.h (expr_type_first_operand_type_p): Declare
	* tree.c (expr_type_first_operand_type_p): New function.
	* ipa-prop.h (ipa_get_type): Allow i to be out of bounds.
	(ipa_value_from_jfunc): Adjust declaration.
	* ipa-cp.c (ipa_get_jf_pass_through_result): New parameter RES_TYPE.
	Use it as result type for arithmetics, unless it is NULL in which case
	be more conservative.
	(ipa_value_from_jfunc): New parameter PARM_TYPE, pass it to
	ipa_get_jf_pass_through_result.
	(propagate_vals_across_pass_through): Likewise.
	(propagate_scalar_across_jump_function): New parameter PARM_TYPE, pass
	is to propagate_vals_across_pass_through.
	(propagate_constants_across_call): Pass PARM_TYPE to
	propagate_scalar_across_jump_function.
	(find_more_scalar_values_for_callers_subset): Pass parameter type to
	ipa_value_from_jfunc.
	(cgraph_edge_brings_all_scalars_for_node): Likewise.
	* ipa-fnsummary.c (evaluate_properties_for_edge): Renamed parms_info
	to caller_parms_info, pass parameter type to ipa_value_from_jfunc.
	* ipa-prop.c (try_make_edge_direct_simple_call): New parameter
	target_type, pass it to ipa_value_from_jfunc.
	(update_indirect_edges_after_inlining): Pass parameter type to
	try_make_edge_direct_simple_call.

testsuite/
	* gcc.dg/ipa/pr82808.c: New test.


Added:
    trunk/gcc/testsuite/gcc.dg/ipa/pr82808.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-cp.c
    trunk/gcc/ipa-fnsummary.c
    trunk/gcc/ipa-prop.c
    trunk/gcc/ipa-prop.h
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree.c
    trunk/gcc/tree.h

Author: jamborm
Date: Wed Nov 29 22:13:34 2017
New Revision: 255256

URL: https://gcc.gnu.org/viewcvs?rev=255256&root=gcc&view=rev
Log:
Be conservative with arithmetic jmp-function types

2017-11-29  Martin Jambor  <mjambor@suse.cz>

	PR ipa/82808
	* tree.c (expr_type_first_operand_type_p): New function.
	* tree.h (expr_type_first_operand_type_p): Declare it.
	* ipa-cp.c (ipa_get_jf_pass_through_result): Use it.

testsuite/
	* gcc.dg/ipa/pr82808.c: New test.


Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.dg/ipa/pr82808.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/ipa-cp.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
    branches/gcc-7-branch/gcc/tree.c
    branches/gcc-7-branch/gcc/tree.h

Fixed.