| Message ID | d6eefa7b-b5c1-d09d-c69b-f3a2584ce8b6@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed,PR,tree-optimization/88797] Thottle back path splitting in another case where it'll inhibit if-conversion | expand |
On Wed, May 1, 2019 at 7:32 PM Jeff Law <law@redhat.com> wrote: > > > This fixes pr88797 by avoiding path splitting when we've got a > candidate, but the PHI feeds a conditional in the join block. ie: > > # iftmp.0_11 = PHI <1111(3), 1112(4)> > [ ... ] > _14 = iftmp.0_11 > x_17; > > > These are more likely going to be if-conversion candidates and > if-conversion is generally more profitable than path splitting. > > This doesn't feel terribly important to fix for gcc-9, so I'm just > installing on the trunk. But backporting would be trivial and safe if > someone feels it's important enough to do so. > > This has been bootstrapped and regression tested on a variety of native > targets, it's also been tested to a lesser degree on the various *-elf > targets. > > Installing on the trunk momentarily. IMHO we should get rid of path splitting and try to integrate its transform with (backward) threading somehow. Richard. > > jeff
On 5/2/19 6:22 AM, Richard Biener wrote: > On Wed, May 1, 2019 at 7:32 PM Jeff Law <law@redhat.com> wrote: >> >> >> This fixes pr88797 by avoiding path splitting when we've got a >> candidate, but the PHI feeds a conditional in the join block. ie: >> >> # iftmp.0_11 = PHI <1111(3), 1112(4)> >> [ ... ] >> _14 = iftmp.0_11 > x_17; >> >> >> These are more likely going to be if-conversion candidates and >> if-conversion is generally more profitable than path splitting. >> >> This doesn't feel terribly important to fix for gcc-9, so I'm just >> installing on the trunk. But backporting would be trivial and safe if >> someone feels it's important enough to do so. >> >> This has been bootstrapped and regression tested on a variety of native >> targets, it's also been tested to a lesser degree on the various *-elf >> targets. >> >> Installing on the trunk momentarily. > > IMHO we should get rid of path splitting and try to integrate its transform > with (backward) threading somehow. BUt that wouldn't change the fundamental problem that we don't have a good way to model costs/benefits. What would be slick would be to create two loops, then do a final selection on which to keep after if-conversion or something along those lines. jeff
On Thu, May 2, 2019 at 3:17 PM Jeff Law <law@redhat.com> wrote: > > On 5/2/19 6:22 AM, Richard Biener wrote: > > On Wed, May 1, 2019 at 7:32 PM Jeff Law <law@redhat.com> wrote: > >> > >> > >> This fixes pr88797 by avoiding path splitting when we've got a > >> candidate, but the PHI feeds a conditional in the join block. ie: > >> > >> # iftmp.0_11 = PHI <1111(3), 1112(4)> > >> [ ... ] > >> _14 = iftmp.0_11 > x_17; > >> > >> > >> These are more likely going to be if-conversion candidates and > >> if-conversion is generally more profitable than path splitting. > >> > >> This doesn't feel terribly important to fix for gcc-9, so I'm just > >> installing on the trunk. But backporting would be trivial and safe if > >> someone feels it's important enough to do so. > >> > >> This has been bootstrapped and regression tested on a variety of native > >> targets, it's also been tested to a lesser degree on the various *-elf > >> targets. > >> > >> Installing on the trunk momentarily. > > > > IMHO we should get rid of path splitting and try to integrate its transform > > with (backward) threading somehow. > BUt that wouldn't change the fundamental problem that we don't have a > good way to model costs/benefits. True. But here we seem to be adding more and more cases where we do _not_ want the transform to a pass initially having not a single case where we _do_ want it... :/ > What would be slick would be to create two loops, then do a final > selection on which to keep after if-conversion or something along those > lines. Of course this scheme quickly explodes it you apply it to more than one transform ;) For optimizations like loop unrolling or jump threading I've always wanted to use the --params we have to limit growth and simply do "code generation" of the paths with doing VN on the fly, simply stopping after those N instructions. That would make it constant time but allow possibly large paths to be handled if they simplify significantly. The main issue is context for the VN process for the path entry which is of course either not present (cheap) or expensive to compute. Richard. > jeff
On 5/2/19 7:23 AM, Richard Biener wrote: > On Thu, May 2, 2019 at 3:17 PM Jeff Law <law@redhat.com> wrote: >> >> On 5/2/19 6:22 AM, Richard Biener wrote: >>> On Wed, May 1, 2019 at 7:32 PM Jeff Law <law@redhat.com> wrote: >>>> >>>> >>>> This fixes pr88797 by avoiding path splitting when we've got a >>>> candidate, but the PHI feeds a conditional in the join block. ie: >>>> >>>> # iftmp.0_11 = PHI <1111(3), 1112(4)> >>>> [ ... ] >>>> _14 = iftmp.0_11 > x_17; >>>> >>>> >>>> These are more likely going to be if-conversion candidates and >>>> if-conversion is generally more profitable than path splitting. >>>> >>>> This doesn't feel terribly important to fix for gcc-9, so I'm just >>>> installing on the trunk. But backporting would be trivial and safe if >>>> someone feels it's important enough to do so. >>>> >>>> This has been bootstrapped and regression tested on a variety of native >>>> targets, it's also been tested to a lesser degree on the various *-elf >>>> targets. >>>> >>>> Installing on the trunk momentarily. >>> >>> IMHO we should get rid of path splitting and try to integrate its transform >>> with (backward) threading somehow. >> BUt that wouldn't change the fundamental problem that we don't have a >> good way to model costs/benefits. > > True. But here we seem to be adding more and more cases where we do > _not_ want the transform to a pass initially having not a single case > where we _do_ want it... :/ Actually we do have cases in the testsuite which show examples of where we do want it (from coremark IIRC). I don't think we've done much in here over the last couple years. I think all I've done is add the half-diamond ifcvt check and the ternary stuff this week. If I was to pick one thing to do it would be to reorganize the ifcvt code a so that we could query it and use the results of that query to throttle back path splitting. What we're doing in the path splitting code to detect ifcvt opportunities is lame/bandaids. But I think we have bigger issues on our plate than this stuff :-) > >> What would be slick would be to create two loops, then do a final >> selection on which to keep after if-conversion or something along those >> lines. > > Of course this scheme quickly explodes it you apply it to more than one > transform ;) Of course. Note that the ability to query and filter out ifcvt opportunities would cut down on the potential explosion here too. > > For optimizations like loop unrolling or jump threading I've always > wanted to use the > --params we have to limit growth and simply do "code generation" of the paths > with doing VN on the fly, simply stopping after those N instructions. Yea. Or some kind of explore code generation approaches and prune. But that feels all pie in the sky to me. Jeff
On Thu, May 2, 2019 at 5:07 PM Jeff Law <law@redhat.com> wrote: > > On 5/2/19 7:23 AM, Richard Biener wrote: > > On Thu, May 2, 2019 at 3:17 PM Jeff Law <law@redhat.com> wrote: > >> > >> On 5/2/19 6:22 AM, Richard Biener wrote: > >>> On Wed, May 1, 2019 at 7:32 PM Jeff Law <law@redhat.com> wrote: > >>>> > >>>> > >>>> This fixes pr88797 by avoiding path splitting when we've got a > >>>> candidate, but the PHI feeds a conditional in the join block. ie: > >>>> > >>>> # iftmp.0_11 = PHI <1111(3), 1112(4)> > >>>> [ ... ] > >>>> _14 = iftmp.0_11 > x_17; > >>>> > >>>> > >>>> These are more likely going to be if-conversion candidates and > >>>> if-conversion is generally more profitable than path splitting. > >>>> > >>>> This doesn't feel terribly important to fix for gcc-9, so I'm just > >>>> installing on the trunk. But backporting would be trivial and safe if > >>>> someone feels it's important enough to do so. > >>>> > >>>> This has been bootstrapped and regression tested on a variety of native > >>>> targets, it's also been tested to a lesser degree on the various *-elf > >>>> targets. > >>>> > >>>> Installing on the trunk momentarily. > >>> > >>> IMHO we should get rid of path splitting and try to integrate its transform > >>> with (backward) threading somehow. > >> BUt that wouldn't change the fundamental problem that we don't have a > >> good way to model costs/benefits. > > > > True. But here we seem to be adding more and more cases where we do > > _not_ want the transform to a pass initially having not a single case > > where we _do_ want it... :/ > Actually we do have cases in the testsuite which show examples of where > we do want it (from coremark IIRC). > > I don't think we've done much in here over the last couple years. I > think all I've done is add the half-diamond ifcvt check and the ternary > stuff this week. > > If I was to pick one thing to do it would be to reorganize the ifcvt > code a so that we could query it and use the results of that query to > throttle back path splitting. What we're doing in the path splitting > code to detect ifcvt opportunities is lame/bandaids. > > But I think we have bigger issues on our plate than this stuff :-) > > > > > > > >> What would be slick would be to create two loops, then do a final > >> selection on which to keep after if-conversion or something along those > >> lines. > > > > Of course this scheme quickly explodes it you apply it to more than one > > transform ;) > Of course. Note that the ability to query and filter out ifcvt > opportunities would cut down on the potential explosion here too. > > > > > For optimizations like loop unrolling or jump threading I've always > > wanted to use the > > --params we have to limit growth and simply do "code generation" of the paths > > with doing VN on the fly, simply stopping after those N instructions. > Yea. Or some kind of explore code generation approaches and prune. But > that feels all pie in the sky to me. True ;) I hope to have some time experimenting with this for unrolling though. Richard. > > Jeff
diff --git a/gcc/gimple-ssa-split-paths.c b/gcc/gimple-ssa-split-paths.c index 33bbb66674b..5bf45eeac28 100644 --- a/gcc/gimple-ssa-split-paths.c +++ b/gcc/gimple-ssa-split-paths.c @@ -264,8 +264,12 @@ is_feasible_trace (basic_block bb) if (is_gimple_debug (stmt)) continue; /* If there's a use in the joiner this might be a CSE/DCE - opportunity. */ - if (gimple_bb (stmt) == bb) + opportunity, but not if the use is in a conditional + which makes this a likely if-conversion candidate. */ + if (gimple_bb (stmt) == bb + && (!is_gimple_assign (stmt) + || (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt)) + != tcc_comparison))) { found_useful_phi = true; break; diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr88797.C b/gcc/testsuite/g++.dg/tree-ssa/pr88797.C new file mode 100644 index 00000000000..75391d6c049 --- /dev/null +++ b/gcc/testsuite/g++.dg/tree-ssa/pr88797.C @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -fdump-tree-split-paths-details" } */ + + +void use(unsigned); +bool f(unsigned x, unsigned y) { + return x < 1111 + (y <= 2222); +} +void test_f(unsigned x, unsigned y) { + for (unsigned i = 0; i < 3333; ++i) + use(f(x++, y++)); +} + +/* { dg-final { scan-tree-dump-not "Duplicating join block" "split-paths" } } */ +/* { dg-final { scan-tree-dump-times "Block . is a join that does not expose" 1 "split-paths" } } */ +