Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
On Sat, Nov 22, 2014 at 07:36:59PM +0100, Richard Biener wrote: On November 22, 2014 12:45:58 PM CET, Jakub Jelinek ja...@redhat.com wrote: On Sat, Nov 22, 2014 at 12:09:46PM +0100, Martin Jambor wrote: 2014-11-21 Martin Jambor mjam...@suse.cz PR ipa/63551 * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert value of the argument to the type of the value in the condition. testsuite/ * gcc.dg/ipa/pr63551.c: New test. Index: src/gcc/ipa-inline-analysis.c === --- src.orig/gcc/ipa-inline-analysis.c +++ src/gcc/ipa-inline-analysis.c @@ -880,7 +880,10 @@ evaluate_conditions_for_known_args (stru } if (c-code == IS_NOT_CONSTANT || c-code == CHANGED) continue; - res = fold_binary_to_constant (c-code, boolean_type_node, val, c-val); + val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c-val), val); VCE should only be used if the sizes of the types are the same. Is that always the case here? I hope so. But I also think it will simply not fold otherwise? Unfortunately, neither is really the case. I have modified the testcase so that the union view_converts an unsigned long to a structure of two signed shorts and sure enough, the code ended up folding a VIEW_CONVERT_EXPR of (unsigned long) -1 to signed short and did that successfully, getting the value of signed short -1. Should I add an extra check to make sure the type sizes match? Thanks, Martin
Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
On November 24, 2014 7:12:01 PM CET, Martin Jambor mjam...@suse.cz wrote: On Sat, Nov 22, 2014 at 07:36:59PM +0100, Richard Biener wrote: On November 22, 2014 12:45:58 PM CET, Jakub Jelinek ja...@redhat.com wrote: On Sat, Nov 22, 2014 at 12:09:46PM +0100, Martin Jambor wrote: 2014-11-21 Martin Jambor mjam...@suse.cz PR ipa/63551 * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert value of the argument to the type of the value in the condition. testsuite/ * gcc.dg/ipa/pr63551.c: New test. Index: src/gcc/ipa-inline-analysis.c === --- src.orig/gcc/ipa-inline-analysis.c +++ src/gcc/ipa-inline-analysis.c @@ -880,7 +880,10 @@ evaluate_conditions_for_known_args (stru } if (c-code == IS_NOT_CONSTANT || c-code == CHANGED) continue; - res = fold_binary_to_constant (c-code, boolean_type_node, val, c-val); + val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c-val), val); VCE should only be used if the sizes of the types are the same. Is that always the case here? I hope so. But I also think it will simply not fold otherwise? Unfortunately, neither is really the case. I have modified the testcase so that the union view_converts an unsigned long to a structure of two signed shorts and sure enough, the code ended up folding a VIEW_CONVERT_EXPR of (unsigned long) -1 to signed short and did that successfully, getting the value of signed short -1. Should I add an extra check to make sure the type sizes match? I would rather say you need to figure out how you end up not rejecting this during propagation. I suppose only the low part will be handled correctly (thus it will fail with a less uniform value either on big or on little-endian). I don't know the IPA code good enough to tell whether you need a size check or whether that would be enough. Sure it Is safer than no size check and I suppose fixing this even more can be done as follow-up. Thus the VIEW_CONVERT patch is still OK. Richard. Thanks, Martin
Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
Hi,
On Fri, Nov 21, 2014 at 09:18:03PM +0100, Richard Biener wrote:
On November 21, 2014 9:07:50 PM CET, Martin Jambor mjam...@suse.cz wrote:
the testcase of PR 63551 passes a union between a signed and an
unsigned integer between two functions as a parameter. The caller
initializes to an unsigned integer with the highest order bit set, the
callee loads the data through the signed field and compares with zero.
evaluate_conditions_for_known_args then wrongly evaluated the
condition in these circumstances, which later on lead to insertion of
builtin_unreachable and miscompilation.
Fixed by fold_converting the known value first. I use the type of the
value in the condition which should do exactly the right thing because
the value is taken from the corresponding gimple_cond statement in
which types must match.
Bootstrapped and tested on x86_64-linux. OK for trunk?
I think you want to use fold_unary (VIEW_CONVERT,...) Here if you consider
the case with
Int and float. And fail if that returns NULL or not a constant.
You are of course right. The following does exactly that.
Bootstrapped and tested on x86_64-linux on trunk and the 4.9 branch.
OK for both?
Thanks,
Martin
2014-11-21 Martin Jambor mjam...@suse.cz
PR ipa/63551
* ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert
value of the argument to the type of the value in the condition.
testsuite/
* gcc.dg/ipa/pr63551.c: New test.
Index: src/gcc/ipa-inline-analysis.c
===
--- src.orig/gcc/ipa-inline-analysis.c
+++ src/gcc/ipa-inline-analysis.c
@@ -880,7 +880,10 @@ evaluate_conditions_for_known_args (stru
}
if (c-code == IS_NOT_CONSTANT || c-code == CHANGED)
continue;
- res = fold_binary_to_constant (c-code, boolean_type_node, val, c-val);
+ val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c-val), val);
+ res = val
+ ? fold_binary_to_constant (c-code, boolean_type_node, val, c-val)
+ : NULL;
if (res integer_zerop (res))
continue;
clause |= 1 (i + predicate_first_dynamic_condition);
Index: src/gcc/testsuite/gcc.dg/ipa/pr63551.c
===
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/ipa/pr63551.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options -Os } */
+
+union U
+{
+ unsigned int f0;
+ int f1;
+};
+
+int a, d;
+
+void
+fn1 (union U p)
+{
+ if (p.f1 = 0)
+if (a)
+ d = 0;
+}
+
+void
+fn2 ()
+{
+ d = 0;
+ union U b = { 4294967286 };
+ fn1 (b);
+}
+
+int
+main ()
+{
+ fn2 ();
+ return 0;
+}
Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
On Sat, Nov 22, 2014 at 12:09:46PM +0100, Martin Jambor wrote: 2014-11-21 Martin Jambor mjam...@suse.cz PR ipa/63551 * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert value of the argument to the type of the value in the condition. testsuite/ * gcc.dg/ipa/pr63551.c: New test. Index: src/gcc/ipa-inline-analysis.c === --- src.orig/gcc/ipa-inline-analysis.c +++ src/gcc/ipa-inline-analysis.c @@ -880,7 +880,10 @@ evaluate_conditions_for_known_args (stru } if (c-code == IS_NOT_CONSTANT || c-code == CHANGED) continue; - res = fold_binary_to_constant (c-code, boolean_type_node, val, c-val); + val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c-val), val); VCE should only be used if the sizes of the types are the same. Is that always the case here? Jakub
Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
On November 22, 2014 12:45:58 PM CET, Jakub Jelinek ja...@redhat.com wrote: On Sat, Nov 22, 2014 at 12:09:46PM +0100, Martin Jambor wrote: 2014-11-21 Martin Jambor mjam...@suse.cz PR ipa/63551 * ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert value of the argument to the type of the value in the condition. testsuite/ * gcc.dg/ipa/pr63551.c: New test. Index: src/gcc/ipa-inline-analysis.c === --- src.orig/gcc/ipa-inline-analysis.c +++ src/gcc/ipa-inline-analysis.c @@ -880,7 +880,10 @@ evaluate_conditions_for_known_args (stru } if (c-code == IS_NOT_CONSTANT || c-code == CHANGED) continue; - res = fold_binary_to_constant (c-code, boolean_type_node, val, c-val); + val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c-val), val); VCE should only be used if the sizes of the types are the same. Is that always the case here? I hope so. But I also think it will simply not fold otherwise? Richard. Jakub
[PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
Hi,
the testcase of PR 63551 passes a union between a signed and an
unsigned integer between two functions as a parameter. The caller
initializes to an unsigned integer with the highest order bit set, the
callee loads the data through the signed field and compares with zero.
evaluate_conditions_for_known_args then wrongly evaluated the
condition in these circumstances, which later on lead to insertion of
builtin_unreachable and miscompilation.
Fixed by fold_converting the known value first. I use the type of the
value in the condition which should do exactly the right thing because
the value is taken from the corresponding gimple_cond statement in
which types must match.
Bootstrapped and tested on x86_64-linux. OK for trunk?
Thanks,
Martin
2014-11-21 Martin Jambor mjam...@suse.cz
PR ipa/63551
* ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert
value of the argument to the type of the value in the condition.
testsuite/
* gcc.dg/ipa/pr63551.c: New test.
Index: src/gcc/ipa-inline-analysis.c
===
--- src.orig/gcc/ipa-inline-analysis.c
+++ src/gcc/ipa-inline-analysis.c
@@ -880,6 +880,7 @@ evaluate_conditions_for_known_args (stru
}
if (c-code == IS_NOT_CONSTANT || c-code == CHANGED)
continue;
+ val = fold_convert (TREE_TYPE (c-val), val);
res = fold_binary_to_constant (c-code, boolean_type_node, val, c-val);
if (res integer_zerop (res))
continue;
Index: src/gcc/testsuite/gcc.dg/ipa/pr63551.c
===
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/ipa/pr63551.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options -Os } */
+
+union U
+{
+ unsigned int f0;
+ int f1;
+};
+
+int a, d;
+
+void
+fn1 (union U p)
+{
+ if (p.f1 = 0)
+if (a)
+ d = 0;
+}
+
+void
+fn2 ()
+{
+ d = 0;
+ union U b = { 4294967286 };
+ fn1 (b);
+}
+
+int
+main ()
+{
+ fn2 ();
+ return 0;
+}
Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
On Fri, Nov 21, 2014 at 09:07:50PM +0100, Martin Jambor wrote:
Hi,
the testcase of PR 63551 passes a union between a signed and an
unsigned integer between two functions as a parameter. The caller
initializes to an unsigned integer with the highest order bit set, the
callee loads the data through the signed field and compares with zero.
evaluate_conditions_for_known_args then wrongly evaluated the
condition in these circumstances, which later on lead to insertion of
builtin_unreachable and miscompilation.
Fixed by fold_converting the known value first. I use the type of the
value in the condition which should do exactly the right thing because
the value is taken from the corresponding gimple_cond statement in
which types must match.
Bootstrapped and tested on x86_64-linux. OK for trunk?
I forgot, this is also a 4.9 bug and I have bootstrapped and tested it
on top of the 4.9 branch as well. So OK for trunk and the 4.9 branch?
Thanks,
Martin
2014-11-21 Martin Jambor mjam...@suse.cz
PR ipa/63551
* ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert
value of the argument to the type of the value in the condition.
testsuite/
* gcc.dg/ipa/pr63551.c: New test.
Index: src/gcc/ipa-inline-analysis.c
===
--- src.orig/gcc/ipa-inline-analysis.c
+++ src/gcc/ipa-inline-analysis.c
@@ -880,6 +880,7 @@ evaluate_conditions_for_known_args (stru
}
if (c-code == IS_NOT_CONSTANT || c-code == CHANGED)
continue;
+ val = fold_convert (TREE_TYPE (c-val), val);
res = fold_binary_to_constant (c-code, boolean_type_node, val,
c-val);
if (res integer_zerop (res))
continue;
Index: src/gcc/testsuite/gcc.dg/ipa/pr63551.c
===
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/ipa/pr63551.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options -Os } */
+
+union U
+{
+ unsigned int f0;
+ int f1;
+};
+
+int a, d;
+
+void
+fn1 (union U p)
+{
+ if (p.f1 = 0)
+if (a)
+ d = 0;
+}
+
+void
+fn2 ()
+{
+ d = 0;
+ union U b = { 4294967286 };
+ fn1 (b);
+}
+
+int
+main ()
+{
+ fn2 ();
+ return 0;
+}
Re: [PATCH, PR 63551] Use proper type in evaluate_conditions_for_known_args
On November 21, 2014 9:07:50 PM CET, Martin Jambor mjam...@suse.cz wrote:
Hi,
the testcase of PR 63551 passes a union between a signed and an
unsigned integer between two functions as a parameter. The caller
initializes to an unsigned integer with the highest order bit set, the
callee loads the data through the signed field and compares with zero.
evaluate_conditions_for_known_args then wrongly evaluated the
condition in these circumstances, which later on lead to insertion of
builtin_unreachable and miscompilation.
Fixed by fold_converting the known value first. I use the type of the
value in the condition which should do exactly the right thing because
the value is taken from the corresponding gimple_cond statement in
which types must match.
Bootstrapped and tested on x86_64-linux. OK for trunk?
I think you want to use fold_unary (VIEW_CONVERT,...) Here if you consider the
case with
Int and float. And fail if that returns NULL or not a constant.
Thanks,
Richard.
Thanks,
Martin
2014-11-21 Martin Jambor mjam...@suse.cz
PR ipa/63551
* ipa-inline-analysis.c (evaluate_conditions_for_known_args): Convert
value of the argument to the type of the value in the condition.
testsuite/
* gcc.dg/ipa/pr63551.c: New test.
Index: src/gcc/ipa-inline-analysis.c
===
--- src.orig/gcc/ipa-inline-analysis.c
+++ src/gcc/ipa-inline-analysis.c
@@ -880,6 +880,7 @@ evaluate_conditions_for_known_args (stru
}
if (c-code == IS_NOT_CONSTANT || c-code == CHANGED)
continue;
+ val = fold_convert (TREE_TYPE (c-val), val);
res = fold_binary_to_constant (c-code, boolean_type_node, val,
c-val);
if (res integer_zerop (res))
continue;
Index: src/gcc/testsuite/gcc.dg/ipa/pr63551.c
===
--- /dev/null
+++ src/gcc/testsuite/gcc.dg/ipa/pr63551.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options -Os } */
+
+union U
+{
+ unsigned int f0;
+ int f1;
+};
+
+int a, d;
+
+void
+fn1 (union U p)
+{
+ if (p.f1 = 0)
+if (a)
+ d = 0;
+}
+
+void
+fn2 ()
+{
+ d = 0;
+ union U b = { 4294967286 };
+ fn1 (b);
+}
+
+int
+main ()
+{
+ fn2 ();
+ return 0;
+}
