PHI-OPT could be rewritten so that it uses fold_ternary (type, COND_EXPR, cond, op1, op2) and that it could remove code which is already done in fold_ternary. A patch like http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00153.html will work but it needs to be reworked for the changes which made it into 4.1.
I am going to do this.
No longer working on this, I am too busy working on the gfortran front-end.
Subject: Bug 25290 Author: rguenth Date: Wed Feb 27 13:17:17 2008 New Revision: 132710 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=132710 Log: 2008-02-27 Richard Guenther <rguenther@suse.de> PR middle-end/25290 * fold-const.c (fold_unary): Return the correct argument, converted to the result type. * gcc.c-torture/execute/pr35390.c: New testcase. Added: trunk/gcc/testsuite/gcc.c-torture/execute/pr35390.c Modified: trunk/gcc/ChangeLog trunk/gcc/fold-const.c trunk/gcc/testsuite/ChangeLog
I actually have a patch to do this but only for the late PHIOPT as it keeps around cond_expr.
Maybe not fold but rather what I am working on, gimple_combine.
I assume it would help with this? int f(int a,int b){ return (a<0)&(b<0); } int g(int a,int b){ return (a<0)?(b<0):0; } int h(int a,int b){ if (a<0) return (b<0); return 0; } where (on x86) we turn g into f, but we don't notice that h is the same. In asm, that is: movl %esi, %eax andl %edi, %eax shrl $31, %eax ((a<0)&(b<0) is later optimized to (a&b)<0) vs shrl $31, %esi xorl %eax, %eax testl %edi, %edi cmovs %esi, %eax
(In reply to Andrew Pinski from comment #5) > Maybe not fold but rather what I am working on, gimple_combine. Rather now gimple_simplify (which is from the match-and-simplify branch).
(In reply to Andrew Pinski from comment #7) > (In reply to Andrew Pinski from comment #5) > > Maybe not fold but rather what I am working on, gimple_combine. > > Rather now gimple_simplify (which is from the match-and-simplify branch). Though I am running into an issue with this. See https://gcc.gnu.org/ml/gcc/2014-11/msg00055.html .
Created attachment 40185 [details] The start of the patch Note this is just a start of the rewrite. It needs improvement as it started out really part of another patch which added a generic (non-loop based) ifcvt to the tree level. But I wanted to get it recorded somewhere so I don't lose it again. Note the problem I mentioned in comment #8 I don't hit any more.
Note this needs at least: https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02837.html And a few more match.pd changes to fully replace conditional_replacement. I hope to be able to get a full patch for the next stage 1.
(In reply to Andrew Pinski from comment #10) > Note this needs at least: > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02837.html > This was approved; has it been committed yet?
(In reply to Eric Gallager from comment #11) > (In reply to Andrew Pinski from comment #10) > > Note this needs at least: > > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02837.html > > > > This was approved; has it been committed yet? Yes. That patch was applied. But that patch is only a support patch and not the full patch to fix this. I was actually going to working on this again but it won't go in until gcc 9.
On February 21, 2018 2:13:22 PM GMT+01:00, "egallager at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> wrote: >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25290 > >Eric Gallager <egallager at gcc dot gnu.org> changed: > > What |Removed |Added >---------------------------------------------------------------------------- > Keywords| |patch > CC| |egallager at gcc dot gnu.org > >--- Comment #11 from Eric Gallager <egallager at gcc dot gnu.org> --- >(In reply to Andrew Pinski from comment #10) >> Note this needs at least: >> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02837.html >> > >This was approved; has it been committed yet? AFAIK no
(In reply to Andrew Pinski from comment #12) > (In reply to Eric Gallager from comment #11) > > (In reply to Andrew Pinski from comment #10) > > > Note this needs at least: > > > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02837.html > > > > > > > This was approved; has it been committed yet? > > Yes. That patch was applied. But that patch is only a support patch and not > the full patch to fix this. I was actually going to working on this again > but it won't go in until gcc 9. It's gcc 9 now.
(In reply to rguenther@suse.de from comment #13) > On February 21, 2018 2:13:22 PM GMT+01:00, "egallager at gcc dot gnu.org" > <gcc-bugzilla@gcc.gnu.org> wrote: > >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25290 > > > >Eric Gallager <egallager at gcc dot gnu.org> changed: > > > > What |Removed |Added > >---------------------------------------------------------------------------- > > Keywords| |patch > > CC| |egallager at gcc dot gnu.org > > > >--- Comment #11 from Eric Gallager <egallager at gcc dot gnu.org> --- > >(In reply to Andrew Pinski from comment #10) > >> Note this needs at least: > >> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02837.html > >> > > > >This was approved; has it been committed yet? > > > AFAIK no Just for refernce, the above patch (which does not fix this bug but was needed as GCC would crash without it) was committed as revision r242970 (or g:28ea3e977ce07c1d0ad14c188336419288fce8d1 ).
I Have a new patch though I need to remove some code still.
The master branch has been updated by Andrew Pinski <pinskia@gcc.gnu.org>: https://gcc.gnu.org/g:9f55df63154a39d67ef5b24def7044bf87300831 commit r12-1152-g9f55df63154a39d67ef5b24def7044bf87300831 Author: Andrew Pinski <apinski@marvell.com> Date: Tue Jun 1 01:05:09 2021 +0000 Replace conditional_replacement with match and simplify This is the first of series of patches to simplify phi-opt to use match and simplify in many cases. This simplification will more things to optimize. This is what Richard requested in https://gcc.gnu.org/pipermail/gcc-patches/2021-May/571197.html and I think it is the right thing to do too. OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. gcc/ChangeLog: PR tree-optimization/25290 * tree-ssa-phiopt.c (match_simplify_replacement): New function. (tree_ssa_phiopt_worker): Use match_simplify_replacement. (two_value_replacement): Change the comment about conditional_replacement. (conditional_replacement): Delete.
(In reply to CVS Commits from comment #17) Note this is the only start of the patches, this is not fully fixed, it is being worked on in a series of patches rather than one big one.
First patch for this was committed as r12-1152. The second patch for this was posted at https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571800.html .
The master branch has been updated by Andrew Pinski <pinskia@gcc.gnu.org>: https://gcc.gnu.org/g:c4574d23cb07340918793a5a98ae7bb2988b3791 commit r12-1309-gc4574d23cb07340918793a5a98ae7bb2988b3791 Author: Andrew Pinski <apinski@marvell.com> Date: Tue Jun 1 06:48:05 2021 +0000 Improve match_simplify_replacement in phi-opt This improves match_simplify_replace in phi-opt to handle the case where there is one cheap (non-call) preparation statement in the middle basic block similar to xor_replacement and others. This allows to remove xor_replacement which it does too. OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. Thanks, Andrew Pinski Changes since v1: v3 - Just minor changes to using gimple_assign_lhs instead of gimple_lhs and fixing a comment. v2 - change the check on the preparation statement to allow only assignments and no calls and only assignments that feed into the phi. gcc/ChangeLog: PR tree-optimization/25290 * tree-ssa-phiopt.c (xor_replacement): Delete. (tree_ssa_phiopt_worker): Delete use of xor_replacement. (match_simplify_replacement): Allow one cheap preparation statement that can be moved to before the if. gcc/testsuite/ChangeLog: * gcc.dg/tree-ssa/pr96928-1.c: Fix testcase for now that ~ happens on the outside of the bit_xor.
Note this is not fully fixed, there is still some more work to do to deal with the non single_non_singleton_phi_for_edges case which will allow to get rid of value_replacement. Note to get rid of early_p check and abs_replacement, we need to add PROP_gimple_lswitch to say we have lowered switches already.
Without load/store handling, here are the following optimizations that either can move to match.pd already or need some extra work to do it: * value_replacement: need to handle !single_non_singleton_phi_for_edges case and more than one feeder statement (2 max according to the current definition) * cond_removal_in_popcount_clz_ctz_pattern: need 2 feeder statements and builtin call handling for feeder statements * two_value_replacement: recored as PR 100958, it can move already * abs_replacement: needs PROP_gimple_lswitch so we don't change if statements early enough ** I think majority of the abs handling is already in match.pd. * minmax_replacement: has some handling of comparisions which might not be in the match.pd patterns already. needs PROP_gimple_lswitch also. ** The handling of: if (a <= u) b = MAX (a, d); x = PHI <b, u> needs to moved too. For the ones which cannot move * factor_out_conditional_conversion: will never move, though it needs improvement and moved already (PR 56223 and PR 13563) * spaceship_replacement: cannot move to match.pd depends on use afterwards which is not hard to deal with in a match pattern.
On Tue, 8 Jun 2021, pinskia at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=25290 > > --- Comment #22 from Andrew Pinski <pinskia at gcc dot gnu.org> --- > * minmax_replacement: has some handling of comparisions which might not be in > the match.pd patterns already. needs PROP_gimple_lswitch also. Yeah, this is also fold-const.c COND_EXPR simplifications using fold_cond_expr_with_comparison (to ABS/MIN/MAX) which are not yet moved to match.pd and are one reason why fold_gimple_assign still dispatches to fold_ternary_loc ... (it shouldn't).
Next patch series can be found here which removes abs_replacement: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573558.html
(In reply to Andrew Pinski from comment #22) So an update on this > * abs_replacement: needs PROP_gimple_lswitch so we don't change if > statements early enough > ** I think majority of the abs handling is already in match.pd. abs_replacement is done and removed :). > * minmax_replacement: has some handling of comparisions which might not be > in the match.pd patterns already. needs PROP_gimple_lswitch also. > ** The handling of: > if (a <= u) > b = MAX (a, d); > x = PHI <b, u> > needs to moved too. > The minmax is recorded as PR 101024. There is some more improvements to gimple_simplify_phiopt needed for early_p as the way min/max patterns are generated in match.pd (extra casts).
(In reply to Andrew Pinski from comment #25) > The minmax is recorded as PR 101024. There is some more improvements to > gimple_simplify_phiopt needed for early_p as the way min/max patterns are > generated in match.pd (extra casts). The early_p part was fixed in r12-2185-g5f2d3ff4e5e2ec .
Created attachment 54813 [details] Set of patches for moving part of the minmax_replacement to match This is my current set of patches for moving minmax_replacement to match.
Created attachment 54821 [details] New set of patches including some other improvements This new set includes moving some of cond_removal_in_builtin_zero_pattern (though not removing it as match-and-simplify phiopt needs to be improved to support 2 defining statements for each bb). It also includes fixing the issue dealing with predicate statements which was missed before. Also reduces the number of patches for the c++ifing by combining some patches together. Note the first patch in the series is an obvious patch which I am going to apply once GCC 13 branches; it removes a prototype which is not needed as gate_hoist_loads is defined before its use.
(In reply to Andrew Pinski from comment #28) > Created attachment 54821 [details] > New set of patches including some other improvements Note this series does not bootstrap, the last patch found that I had a latent bug in the patch that started to handle diamond shaped PHIs. For: if (_16 == 34) goto <bb 10>; [INV] else goto <bb 12>; [INV] <bb 10> : ... _27 = csets__identifier_char[_26]; if (_27 != 0) goto <bb 11>; [INV] else goto <bb 12>; [INV] <bb 11> : // predicted unlikely by early return (on trees) predictor. goto <bb 13>; [INV] <bb 12> : // predicted unlikely by early return (on trees) predictor. <bb 13> : # _28 = PHI <1(7), 0(12), 1(4), 1(11)> I would transform it to use _27 even from bb 12 which is wrong. I had missed that minmax_replacement did: if (!single_pred_p (middle_bb) || !single_pred_p (alt_middle_bb) || !single_succ_p (middle_bb) || !single_succ_p (alt_middle_bb)) return false; I will add that check tomorrow.
Created attachment 54930 [details] New set of patches Note this is a new set, It does not do the C++ify as I had done before. It is missing some of the factor_out_* patches from the previous set; I am re-implementing that (the last patch in the series is the start of that).
So what is left as of May 4th 2023: minmax_replacement: * some min/max detection has not moved to match (cast dealing with INT_MAX) * `if (a < b) c = min/max(a, d)` transformation has not moved to match cond_removal_in_builtin_zero_pattern: * supporting of one extra statement to move out of the conditional (has to be a cast)
https://gcc.gnu.org/pipermail/gcc-patches/2004-June/140297.html for reference where the original idea came from.
(In reply to Marc Glisse from comment #6) > I assume it would help with this? Note that was fixed outside of phiopt in GCC 11 by r11-6609-g13d47c37a2c043 (PR 95731).
(In reply to Andrew Pinski from comment #22) > Without load/store handling, here are the following optimizations that > either can move to match.pd already or need some extra work to do it: > > * value_replacement: need to handle !single_non_singleton_phi_for_edges case > and more than one feeder statement (2 max according to the current > definition) There is a secondary part to value_replacement which requires more work than just working on the phi. I have to think of how to handle that. > * cond_removal_in_popcount_clz_ctz_pattern: need 2 feeder statements and > builtin call handling for feeder statements > > > * two_value_replacement: recored as PR 100958, it can move already Handled. > * abs_replacement: needs PROP_gimple_lswitch so we don't change if > statements early enough > ** I think majority of the abs handling is already in match.pd. This is handled. > * minmax_replacement: has some handling of comparisions which might not be > in the match.pd patterns already. needs PROP_gimple_lswitch also. > ** The handling of: > if (a <= u) > b = MAX (a, d); > x = PHI <b, u> > needs to moved too. > > > For the ones which cannot move There is a new part of minmax which I have a patch for but there is one part missing still (recorded in PR 101024) > * factor_out_conditional_conversion: will never move, though it needs > improvement and moved already (PR 56223 and PR 13563) I have an implementation which moves this over to using gimple_match_op (which I need to a few testcases for it). Though some of this maybe should move over to match ... > * spaceship_replacement: cannot move to match.pd depends on use afterwards > which is not hard to deal with in a match pattern. I have to look at what is done for spaceship and maybe merge it in with what is done for value_replacement (comparison after the phi) since they implement a similar optimization.