Hi,
On Tue, Dec 03 2024, Richard Biener wrote:
> On Tue, Dec 3, 2024 at 12:09 PM Martin Jambor <mjam...@suse.cz> wrote:
>> On Fri, Nov 15 2024, Richard Biener wrote:
>> > On Fri, Nov 15, 2024 at 1:45 PM Jan Hubicka <hubi...@ucw.cz> wrote:
>> >> >
>> >> > The patch only ever skips just one conversion, never a chain of them and
>> [ 12 more citation lines. Click/Enter to show. ]
>> >> > deliberately so, for the reasons shown in the example.
>> >> >
>> >> > However, I have just realized that combining pass_through jump functions
>> >> > during inlining may unfortunately have exactly the same effect, so we
>> >> > indeed need to be more careful.
>> >>
>> >> Ah, right. I was thinking if I can trigger someting like this and this
>> >> option did not came to my mind.
>> >> >
>> >> > The motivating example converts a bool (integer with precision one) to
>> >> > an int, so what about the patch below which allows converting between
>> >> > integers and to a higher precision? (Assuming the patch will pass
>> >> > bootstrap and testing on x86_64-linux which is underway).
>> >>
>> >> Allowing only conversion that monotonously increase precision looks.
>>
>> As in "looks better?" Or perhaps even "looks good?"
>>
>> >> Perhaps Richi may have opinion here.
>> >> In a way this is similar to what we do in gimple_call_builtin that has
>> [ 13 more citation lines. Click/Enter to show. ]
>> >> some type checking and also allows widening conversions. So perhaps
>> >> this can be unified.
>> >>
>> >> I just noticed that testusite has few examples that, for example, define
>> >>
>> >> void *calloc (long, long)
>> >>
>> >> and this makes the test fail since parameter is really unsigned long
>> >> and in the turn we disable some calloc optimizations even though this
>> >> does not affect code generation. Some passes use
>> >> gimple_call_builtin while other look up callee decl by hand.
>> >
>> > I think all conversions that are not value changing (based on incoming
>> > range)
>> > are OK. Even signed int with [3, 10] -> unsigned char [3, 10] would be OK.
>> > But signed int with [-1, 1] -> unsigned char [0, 1] [ 0xff ] might
>> > cause problems.
>>
>> Right, I was not going to use ranges for this because I suspected that
>> more often than not the range would be unknown. But if that's the
>> suggestion, would something like the following (only very mildly tested)
>> function be OK?
>>
>> I kept the type check because it looks quite a bit cheaper and would
>> work also for conversions between integers with the same precision but
>> different signedness which we can safely fold_convert between as well.
>
> Maybe you can use range_fits_type_p? (aka int_fits_type_p for INTEGER_CSTs)
>
> What I was saying is that your change looks OK (but it can be possibly
> improved
> upon).
>
thanks for the suggestion! range_fits_type_p is indeed a function I can
use directly.
I am going to commit the following patch which has been pre-approved by
Honza and which has passed bootstrap and testing on x86_64 and together
with other two patches also on ppc64le-linux and an LTO bootstrap on
x86_64-linux.
Martin
---------- 8< ----------
Originally, we did not stream any formal parameter types into WPA and
were generally very conservative when it came to type mismatches in
IPA-CP. Over the time, mismatches that happen in code and blew up in
WPA made us to be much more resilient and also to stream the types of
the parameters which we now use commonly.
With that information, we can safely skip conversions when looking at
the IL from which we build jump functions and then simply fold convert
the constants and ranges to the resulting type, as long as we are
careful that performing the corresponding folding of constants gives
the corresponding results. In order to do that, we must ensure that
the old value can be represented in the new one without any loss.
With this change, we can nicely propagate non-NULLness in IPA-VR as
demonstrated with the new test case.
I have gone through all other uses of (all components of) jump
functions which could be affected by this and verified they do indeed
check types and can handle mismatches.
gcc/ChangeLog:
2024-12-11 Martin Jambor <mjam...@suse.cz>
* ipa-prop.cc: Include vr-values.h.
(skip_a_safe_conversion_op): New function.
(ipa_compute_jump_functions_for_edge): Use it.
gcc/testsuite/ChangeLog:
2024-11-01 Martin Jambor <mjam...@suse.cz>
* gcc.dg/ipa/vrp9.c: New test.
---
gcc/ipa-prop.cc | 40 +++++++++++++++++++++++++++
gcc/testsuite/gcc.dg/ipa/vrp9.c | 48 +++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/ipa/vrp9.c
diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
index 3d72794e37c..ae309ec78a2 100644
--- a/gcc/ipa-prop.cc
+++ b/gcc/ipa-prop.cc
@@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If not see
#include "attr-fnspec.h"
#include "gimple-range.h"
#include "value-range-storage.h"
+#include "vr-values.h"
/* Function summary where the parameter infos are actually stored. */
ipa_node_params_t *ipa_node_params_sum = NULL;
@@ -2311,6 +2312,44 @@ ipa_set_jfunc_vr (ipa_jump_func *jf, const ipa_vr &vr)
ipa_set_jfunc_vr (jf, tmp);
}
+
+/* If T is an SSA_NAME that is the result of a simple type conversion statement
+ from an integer type to another integer type which is known to be able to
+ represent the values the operand of the conversion can hold, return the
+ operand of that conversion, otherwise return T. */
+
+static tree
+skip_a_safe_conversion_op (tree t)
+{
+ if (TREE_CODE (t) != SSA_NAME
+ || SSA_NAME_IS_DEFAULT_DEF (t))
+ return t;
+
+ gimple *def = SSA_NAME_DEF_STMT (t);
+ if (!is_gimple_assign (def)
+ || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def))
+ || !INTEGRAL_TYPE_P (TREE_TYPE (t))
+ || !INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (def))))
+ return t;
+
+ tree rhs1 = gimple_assign_rhs1 (def);
+ if (TYPE_PRECISION (TREE_TYPE (t))
+ >= TYPE_PRECISION (TREE_TYPE (rhs1)))
+ return gimple_assign_rhs1 (def);
+
+ value_range vr (TREE_TYPE (rhs1));
+ if (!get_range_query (cfun)->range_of_expr (vr, rhs1, def)
+ || vr.undefined_p ())
+ return t;
+
+ irange &ir = as_a <irange> (vr);
+ if (range_fits_type_p (&ir, TYPE_PRECISION (TREE_TYPE (t)),
+ TYPE_SIGN (TREE_TYPE (t))))
+ return gimple_assign_rhs1 (def);
+
+ return t;
+}
+
/* Compute jump function for all arguments of callsite CS and insert the
information in the jump_functions array in the ipa_edge_args corresponding
to this callsite. */
@@ -2415,6 +2454,7 @@ ipa_compute_jump_functions_for_edge (struct
ipa_func_body_info *fbi,
gcc_assert (!jfunc->m_vr);
}
+ arg = skip_a_safe_conversion_op (arg);
if (is_gimple_ip_invariant (arg)
|| (VAR_P (arg)
&& is_global_var (arg)
diff --git a/gcc/testsuite/gcc.dg/ipa/vrp9.c b/gcc/testsuite/gcc.dg/ipa/vrp9.c
new file mode 100644
index 00000000000..461a2e757d2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/vrp9.c
@@ -0,0 +1,48 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+int some_f1 (int);
+int some_f2 (int);
+int some_f3 (int);
+
+void remove_this_call ();
+
+int g;
+
+static int __attribute__((noinline))
+bar (int p)
+{
+ if (p)
+ remove_this_call ();
+ return g++;
+}
+
+static int __attribute__((noinline))
+foo (int (*f)(int))
+{
+ return bar (f == (void *)0);
+}
+
+int
+baz1 (void)
+{
+ int (*f)(int);
+ if (g)
+ f = some_f1;
+ else
+ f = some_f2;
+ return foo (f);
+}
+
+int
+baz2 (void)
+{
+ int (*f)(int);
+ if (g)
+ f = some_f2;
+ else
+ f = some_f3;
+ return foo (f);
+}
+
+/* { dg-final { scan-tree-dump-not "remove_this_call" "optimized" } } */
--
2.47.1
