* PR61140: check the phi is unique in value_replacement
@ 2014-05-10 22:53 Marc Glisse
2014-05-11 0:33 ` Andrew Pinski
0 siblings, 1 reply; 4+ messages in thread
From: Marc Glisse @ 2014-05-10 22:53 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 867 bytes --]
Hello,
in my recent phiopt patch enhancing value_replacement to optimize
x!=0?x+y:y, I forgot to check that there is no other PHI (not sure how I
managed to miss that since I copy-pasted the line just below the test).
If there are other phi nodes (with different arguments for those 2
branches), it would be possible to replace the phi argument and stop there
(as value_replacement does for its other transformation). However, I am
chosing to punt. The cost analysis would be different, and I wrote the
transformation assuming that this single-phi test was already done higher
in the function.
Bootstrap+testsuite on x86_64-linux-gnu.
2014-05-12 Marc Glisse <marc.glisse@inria.fr>
PR tree-optimization/61140
gcc/
* tree-ssa-phiopt.c (value_replacement): Punt on multiple phis.
gcc/testsuite/
* gcc.dg/tree-ssa/pr61140.c: New file.
--
Marc Glisse
[-- Attachment #2: Type: TEXT/PLAIN, Size: 1893 bytes --]
Index: gcc/testsuite/gcc.dg/tree-ssa/pr61140.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr61140.c (revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr61140.c (working copy)
@@ -0,0 +1,18 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+int a[1] = { 1 }, b = 1, c;
+
+int
+main ()
+{
+ for (; c < 1; c++)
+ if (a[0])
+ {
+ a[0] &= 1;
+ b = 0;
+ }
+ if (b)
+ __builtin_abort ();
+ return 0;
+}
Index: gcc/tree-ssa-phiopt.c
===================================================================
--- gcc/tree-ssa-phiopt.c (revision 210301)
+++ gcc/tree-ssa-phiopt.c (working copy)
@@ -842,20 +842,24 @@ value_replacement (basic_block cond_bb,
/* Now optimize (x != 0) ? x + y : y to just y.
The following condition is too restrictive, there can easily be another
stmt in middle_bb, for instance a CONVERT_EXPR for the second argument. */
gimple assign = last_and_only_stmt (middle_bb);
if (!assign || gimple_code (assign) != GIMPLE_ASSIGN
|| gimple_assign_rhs_class (assign) != GIMPLE_BINARY_RHS
|| (!INTEGRAL_TYPE_P (TREE_TYPE (arg0))
&& !POINTER_TYPE_P (TREE_TYPE (arg0))))
return 0;
+ /* Only transform if it removes the condition. */
+ if (!single_non_singleton_phi_for_edges (phi_nodes (gimple_bb (phi)), e0, e1))
+ return 0;
+
/* Size-wise, this is always profitable. */
if (optimize_bb_for_speed_p (cond_bb)
/* The special case is useless if it has a low probability. */
&& profile_status_for_fn (cfun) != PROFILE_ABSENT
&& EDGE_PRED (middle_bb, 0)->probability < PROB_EVEN
/* If assign is cheap, there is no point avoiding it. */
&& estimate_num_insns (assign, &eni_time_weights)
>= 3 * estimate_num_insns (cond, &eni_time_weights))
return 0;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PR61140: check the phi is unique in value_replacement
2014-05-10 22:53 PR61140: check the phi is unique in value_replacement Marc Glisse
@ 2014-05-11 0:33 ` Andrew Pinski
2014-05-11 10:03 ` Marc Glisse
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Pinski @ 2014-05-11 0:33 UTC (permalink / raw)
To: Marc Glisse; +Cc: GCC Patches
On Sat, May 10, 2014 at 3:53 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> in my recent phiopt patch enhancing value_replacement to optimize
> x!=0?x+y:y, I forgot to check that there is no other PHI (not sure how I
> managed to miss that since I copy-pasted the line just below the test).
>
> If there are other phi nodes (with different arguments for those 2
> branches), it would be possible to replace the phi argument and stop there
> (as value_replacement does for its other transformation). However, I am
> chosing to punt. The cost analysis would be different, and I wrote the
> transformation assuming that this single-phi test was already done higher in
> the function.
I think we should have some good cost analysis because for this
testcase, we should be able to get only one conditional move but right
now with punting we don't.
>
> Bootstrap+testsuite on x86_64-linux-gnu.
>
> 2014-05-12 Marc Glisse <marc.glisse@inria.fr>
>
> PR tree-optimization/61140
> gcc/
> * tree-ssa-phiopt.c (value_replacement): Punt on multiple phis.
> gcc/testsuite/
> * gcc.dg/tree-ssa/pr61140.c: New file.
>
> --
> Marc Glisse
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr61140.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr61140.c (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr61140.c (working copy)
> @@ -0,0 +1,18 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +int a[1] = { 1 }, b = 1, c;
> +
> +int
> +main ()
> +{
> + for (; c < 1; c++)
> + if (a[0])
> + {
> + a[0] &= 1;
> + b = 0;
> + }
> + if (b)
> + __builtin_abort ();
> + return 0;
> +}
> Index: gcc/tree-ssa-phiopt.c
> ===================================================================
> --- gcc/tree-ssa-phiopt.c (revision 210301)
> +++ gcc/tree-ssa-phiopt.c (working copy)
> @@ -842,20 +842,24 @@ value_replacement (basic_block cond_bb,
> /* Now optimize (x != 0) ? x + y : y to just y.
> The following condition is too restrictive, there can easily be
> another
> stmt in middle_bb, for instance a CONVERT_EXPR for the second
> argument. */
> gimple assign = last_and_only_stmt (middle_bb);
> if (!assign || gimple_code (assign) != GIMPLE_ASSIGN
> || gimple_assign_rhs_class (assign) != GIMPLE_BINARY_RHS
> || (!INTEGRAL_TYPE_P (TREE_TYPE (arg0))
> && !POINTER_TYPE_P (TREE_TYPE (arg0))))
> return 0;
>
> + /* Only transform if it removes the condition. */
> + if (!single_non_singleton_phi_for_edges (phi_nodes (gimple_bb (phi)), e0,
> e1))
> + return 0;
> +
> /* Size-wise, this is always profitable. */
> if (optimize_bb_for_speed_p (cond_bb)
> /* The special case is useless if it has a low probability. */
> && profile_status_for_fn (cfun) != PROFILE_ABSENT
> && EDGE_PRED (middle_bb, 0)->probability < PROB_EVEN
> /* If assign is cheap, there is no point avoiding it. */
> && estimate_num_insns (assign, &eni_time_weights)
> >= 3 * estimate_num_insns (cond, &eni_time_weights))
> return 0;
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PR61140: check the phi is unique in value_replacement
2014-05-11 0:33 ` Andrew Pinski
@ 2014-05-11 10:03 ` Marc Glisse
2014-05-12 16:25 ` Jeff Law
0 siblings, 1 reply; 4+ messages in thread
From: Marc Glisse @ 2014-05-11 10:03 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sat, 10 May 2014, Andrew Pinski wrote:
> On Sat, May 10, 2014 at 3:53 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> in my recent phiopt patch enhancing value_replacement to optimize
>> x!=0?x+y:y, I forgot to check that there is no other PHI (not sure how I
>> managed to miss that since I copy-pasted the line just below the test).
>>
>> If there are other phi nodes (with different arguments for those 2
>> branches), it would be possible to replace the phi argument and stop there
>> (as value_replacement does for its other transformation). However, I am
>> chosing to punt. The cost analysis would be different, and I wrote the
>> transformation assuming that this single-phi test was already done higher in
>> the function.
>
> I think we should have some good cost analysis because for this
> testcase, we should be able to get only one conditional move but right
> now with punting we don't.
That's true. But note that the transformation is already very limited
(gives up if there is a second statement in the middle bb, even a simple
cast), so I would like to first quickly get the wrong-code regression out
of the way, and we can make improvements afterwards (though we can of
course start discussing them now).
It seems like if there is only 1 extra non-singleton phi (in addition to
the one we are transforming) and the target supports conditional move for
this type and the direct branch has proba < 50%, with the other
restrictions already in place, we could go ahead. How does that sound? Not
too specialized? If there are many phis, conditional moves are out, the
branch will stay, and unless the edge to the operation has a very high
proba, it doesn't seem like a good idea to pull the operation out of the
branch.
--
Marc Glisse
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: PR61140: check the phi is unique in value_replacement
2014-05-11 10:03 ` Marc Glisse
@ 2014-05-12 16:25 ` Jeff Law
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Law @ 2014-05-12 16:25 UTC (permalink / raw)
To: Marc Glisse, Andrew Pinski; +Cc: GCC Patches
On 05/11/14 04:03, Marc Glisse wrote:
> On Sat, 10 May 2014, Andrew Pinski wrote:
>
>> On Sat, May 10, 2014 at 3:53 PM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>> Hello,
>>>
>>> in my recent phiopt patch enhancing value_replacement to optimize
>>> x!=0?x+y:y, I forgot to check that there is no other PHI (not sure how I
>>> managed to miss that since I copy-pasted the line just below the test).
>>>
>>> If there are other phi nodes (with different arguments for those 2
>>> branches), it would be possible to replace the phi argument and stop
>>> there
>>> (as value_replacement does for its other transformation). However, I am
>>> chosing to punt. The cost analysis would be different, and I wrote the
>>> transformation assuming that this single-phi test was already done
>>> higher in
>>> the function.
>>
>> I think we should have some good cost analysis because for this
>> testcase, we should be able to get only one conditional move but right
>> now with punting we don't.
>
> That's true. But note that the transformation is already very limited
> (gives up if there is a second statement in the middle bb, even a simple
> cast), so I would like to first quickly get the wrong-code regression
> out of the way, and we can make improvements afterwards (though we can
> of course start discussing them now).
>
> It seems like if there is only 1 extra non-singleton phi (in addition to
> the one we are transforming) and the target supports conditional move
> for this type and the direct branch has proba < 50%, with the other
> restrictions already in place, we could go ahead. How does that sound?
> Not too specialized? If there are many phis, conditional moves are out,
> the branch will stay, and unless the edge to the operation has a very
> high proba, it doesn't seem like a good idea to pull the operation out
> of the branch.
Your call based on what you see from a codegen standpoint. Having been
burned before with these kinds of transformations, I tend to be a bit
conservative :-)
If you decide to keep things as-is, the patch is fine. If you want to
extend to handle the additional case, please repost for review.
Thanks,
jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-05-12 16:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-10 22:53 PR61140: check the phi is unique in value_replacement Marc Glisse
2014-05-11 0:33 ` Andrew Pinski
2014-05-11 10:03 ` Marc Glisse
2014-05-12 16:25 ` Jeff Law
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).