On Thu, Dec 19, 2024 at 6:53 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
>
> Introduce flags to disable ifcombine as a whole, or its new components.
>
> Disable the potentially quadratic noncontiguous ifcombine at -O1.
> Adjust the tests that expected it with -O to use -O2 instead.
>
> Is this of interest? I've made it mostly for PR118032, but turned it
> into a proper patch because it seems to make some sense to be able to
> disable these more expensive and less stable features selectively.
> Regstrapping on x86_64-linux-gnu. Ok to install?
>
>
> for gcc/ChangeLog
>
> * common.opt (fcombine-conditionals): New.
> (fcombine-field-conditionals): New.
> (fcombine-noncontiguous-conditionals): New.
> * doc/invoke.texi: Document them.
> * tree-ssa-ifcombine.cc (ifcombine_ifandif): Don't call
> fold_truth_andor if -fno-combine-field-conditionals.
> (tree_ssa_ifcombine_bb): Quit after the first attempt under
> -fno-combine-noncontiguous-conditionals.
> (pass_tree_ifcombine::gate): New.
>
> for gcc/testsuite/ChangeLog
>
> * gcc.dg/field-merge-1.c: Bump to -O2.
> * gcc.dg/field-merge-2.c: Likewise.
> * gcc.dg/field-merge-3.c: Likewise.
> * gcc.dg/field-merge-4.c: Likewise.
> * gcc.dg/field-merge-5.c: Likewise.
> * gcc.dg/field-merge-6.c: Likewise.
> * gcc.dg/field-merge-7.c: Likewise.
> * gcc.dg/field-merge-8.c: Likewise.
> * gcc.dg/field-merge-9.c: Likewise.
> * gcc.dg/field-merge-10.c: Likewise.
> * gcc.dg/field-merge-11.c: Likewise.
> * gcc.dg/field-merge-13.c: Likewise.
> * gcc.dg/field-merge-14.c: Likewise.
> * gcc.dg/field-merge-15.c: Likewise.
> * gcc.dg/field-merge-16.c: Likewise.
> ---
> gcc/common.opt | 12 +++++++++++
> gcc/doc/invoke.texi | 30 +++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/field-merge-1.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-10.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-11.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-13.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-14.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-15.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-16.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-2.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-3.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-4.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-5.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-6.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-7.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-8.c | 2 +-
> gcc/testsuite/gcc.dg/field-merge-9.c | 2 +-
> gcc/tree-ssa-ifcombine.cc | 37
> ++++++++++++++++++++++-----------
> 18 files changed, 82 insertions(+), 27 deletions(-)
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 1b72826d44b11..aa5ad79741126 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1241,6 +1241,18 @@ fcode-hoisting
> Common Var(flag_code_hoisting) Optimization
> Enable code hoisting.
>
> +fcombine-conditionals
> +Common Var(flag_tree_ifcombine) Init(-1) Optimization
> +Combine conditionals, with the ifcombine optimization pass.
> +
> +fcombine-field-conditionals
> +Common Var(flag_tree_ifcombine_fieldmerge) Init(-1) Optimization
> +Combine conditionals involving separate fields
> +
> +fcombine-noncontiguous-conditionals
> +Common Var(flag_tree_ifcombine_noncontig) Init(-1) Optimization
> +Combine conditionals from noncontiguous blocks
Please don't use -1 initializers, instead populate
opts.cc:default_options_table.
IMO options where it's not clear how they interact are bad. Does
-fcombine-field-conditionals enable -fcombine-conditionals? Does
-fno-combine-field-conditionals disable it? A
-fcombine-conditionals=<mode> might be more obvious here,
though with two independent features (fields and non-contiguous)
a set of <feature> might be better, but that has no generic option
machinery support.
I'd go for a pragmatic solution, add -fcombine-conditionals and
document that with -fexpensive-optimizations we're enabling
non-contiguous support and a --param to limit it's search depth.
I think we don't need the field-conditionals flag at this point
(fold always did this), if you want one for debugging I suggest
to add a --param for it.
> fcombine-stack-adjustments
> Common Var(flag_combine_stack_adjustments) Optimization
> Looks for opportunities to reduce stack adjustments and stack references.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 8ed5536365f79..317b1dd233a66 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -570,6 +570,8 @@ Objective-C and Objective-C++ Dialects}.
> -fassociative-math -fauto-profile -fauto-profile[=@var{path}]
> -fauto-inc-dec -fbranch-probabilities
> -fcaller-saves
> +-fcombine-conditionals -fcombine-field-conditionals
> +-fcombine-noncontiguous-conditionals
> -fcombine-stack-adjustments -fconserve-stack
> -ffold-mem-offsets
> -fcompare-elim -fcprop-registers -fcrossjumping
> @@ -12633,6 +12635,8 @@ complexity than at @option{-O}.
> @c Please keep the following list alphabetized.
> @gccoptlist{-fauto-inc-dec
> -fbranch-count-reg
> +-fcombine-conditionals
> +-fcombine-field-conditionals
> -fcombine-stack-adjustments
> -fcompare-elim
> -fcprop-registers
> @@ -12693,6 +12697,7 @@ also turns on the following optimization flags:
> @gccoptlist{-falign-functions -falign-jumps
> -falign-labels -falign-loops
> -fcaller-saves
> +-fcombine-noncontiguous-conditionals
> -fcode-hoisting
> -fcrossjumping
> -fcse-follow-jumps -fcse-skip-blocks
> @@ -13676,6 +13681,31 @@ those which have no call-preserved registers to use
> instead.
>
> Enabled at levels @option{-O2}, @option{-O3}, @option{-Os}.
>
> +@opindex fcombine-conditionals
> +@opindex fno-combine-conditionals
> +@item -fcombine-conditionals
> +Enable combination of conditionals from blocks without side effects.
> +
> +It has no effect when not optimizing.
> +
> +@opindex fcombine-field-conditionals
> +@opindex fno-combine-field-conditionals
> +@item -fcombine-field-conditionals
> +Enable combination of conditionals involving different fields, and
> +similar transformations that widen loads to save compares.
> +
> +It has no effect if @option{-fcombine-conditionals} is not enabled.
> +
> +@opindex fcombine-noncontiguous-conditionals
> +@opindex fno-combine-noncontiguous-conditionals
> +@item -fcombine-noncontiguous-conditionals
> +Enable combination of conditionals that belong to the same chain of
> +tests, even if they aren't adjacent.
> +
> +It has no effect if @option{-fcombine-conditionals} is not enabled.
> +
> +Enabled at levels @option{-O2}, @option{-O3}, @option{-Os}.
> +
> @opindex fcombine-stack-adjustments
> @item -fcombine-stack-adjustments
> Tracks stack adjustments (pushes and pops) and stack memory references
> diff --git a/gcc/testsuite/gcc.dg/field-merge-1.c
> b/gcc/testsuite/gcc.dg/field-merge-1.c
> index 1818e104437e1..64905ca04d197 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-1.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-1.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O -save-temps -fdump-tree-optimized" } */
> +/* { dg-options "-O2 -save-temps -fdump-tree-optimized" } */
>
> /* Check that field loads compared with constants are merged, even if
> tested out of order, and when fields straddle across alignment
> diff --git a/gcc/testsuite/gcc.dg/field-merge-10.c
> b/gcc/testsuite/gcc.dg/field-merge-10.c
> index dca6febb75859..5ad33b9068505 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-10.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-10.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> /* Check that we don't move loads across stores. */
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-11.c
> b/gcc/testsuite/gcc.dg/field-merge-11.c
> index fe627cddd7fdf..66af1f0b21b09 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-11.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-11.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> /* Check that narrowing casts aren't ignored, and that same-field tests at
> different widths aren't misoptimized. */
> diff --git a/gcc/testsuite/gcc.dg/field-merge-13.c
> b/gcc/testsuite/gcc.dg/field-merge-13.c
> index 7e4f4c499347f..dc5259b1be8b5 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-13.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-13.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +/* { dg-options "-O2 -fdump-tree-ifcombine-details" } */
>
> /* Check that we optimize swapped compares, and that we don't load from
> objects
> before they're fully initialized. */
> diff --git a/gcc/testsuite/gcc.dg/field-merge-14.c
> b/gcc/testsuite/gcc.dg/field-merge-14.c
> index 91d84cfebf196..0ef0942c176dc 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-14.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-14.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +/* { dg-options "-O2 -fdump-tree-ifcombine-details" } */
>
> /* Check that we don't get confused by multiple conversions. */
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-15.c
> b/gcc/testsuite/gcc.dg/field-merge-15.c
> index 34641e893c92f..e365f8f86c2d1 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-15.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-15.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +/* { dg-options "-O2 -fdump-tree-ifcombine-details" } */
>
> /* Check that bitfield compares-with-zero turned into GT and LE compares with
> powers-of-two minus 1 are optimized. */
> diff --git a/gcc/testsuite/gcc.dg/field-merge-16.c
> b/gcc/testsuite/gcc.dg/field-merge-16.c
> index 2ca23ea663a45..75783547eef66 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-16.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-16.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +/* { dg-options "-O2 -fdump-tree-ifcombine-details" } */
>
> /* Check that tests for sign-extension bits are handled correctly. */
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-2.c
> b/gcc/testsuite/gcc.dg/field-merge-2.c
> index 80c573b31ddc7..976d41e357ad6 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-2.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-2.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> struct TL {
> unsigned short a;
> diff --git a/gcc/testsuite/gcc.dg/field-merge-3.c
> b/gcc/testsuite/gcc.dg/field-merge-3.c
> index f26e8a96ea04f..6ea01007422f4 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-3.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-3.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> const int BIG_ENDIAN_P = (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__);
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-4.c
> b/gcc/testsuite/gcc.dg/field-merge-4.c
> index c629069e52b2c..691f95fe94c82 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-4.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-4.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> struct T1 {
> unsigned int zn;
> diff --git a/gcc/testsuite/gcc.dg/field-merge-5.c
> b/gcc/testsuite/gcc.dg/field-merge-5.c
> index 1580b14bcc935..6fbab1760f0cd 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-5.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-5.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> struct T1 {
> unsigned int zn;
> diff --git a/gcc/testsuite/gcc.dg/field-merge-6.c
> b/gcc/testsuite/gcc.dg/field-merge-6.c
> index 7fd48a138d14a..49465682c1fa1 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-6.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-6.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
> /* { dg-shouldfail } */
>
> /* Check that the third compare won't be pulled ahead of the second one and
> diff --git a/gcc/testsuite/gcc.dg/field-merge-7.c
> b/gcc/testsuite/gcc.dg/field-merge-7.c
> index 728a29b6fafa9..a6a2f0a3bb0e0 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-7.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-7.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +/* { dg-options "-O2 -fdump-tree-ifcombine-details" } */
>
> /* Check that the third compare won't be combined with the first one. */
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-8.c
> b/gcc/testsuite/gcc.dg/field-merge-8.c
> index ae270e10070e4..8d71f919b8977 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-8.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-8.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O2" } */
>
> /* Check that conversions are not thrown away. */
>
> diff --git a/gcc/testsuite/gcc.dg/field-merge-9.c
> b/gcc/testsuite/gcc.dg/field-merge-9.c
> index 04df54c2b74ef..64fdfe3d0c6e8 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-9.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-9.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O -fdump-tree-ifcombine-details" } */
> +/* { dg-options "-O2 -fdump-tree-ifcombine-details" } */
>
> /* Check that conversions and selections of similar bit ranges across
> different
> types don't prevent combination. */
> diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> index c9399a1069452..108fa5b17dacb 100644
> --- a/gcc/tree-ssa-ifcombine.cc
> +++ b/gcc/tree-ssa-ifcombine.cc
> @@ -972,18 +972,19 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool
> inner_inv,
> gimple_cond_lhs (outer_cond),
> gimple_cond_rhs (outer_cond),
> gimple_bb (outer_cond)))
> - && !(t = (fold_truth_andor_for_ifcombine
> - (TRUTH_ANDIF_EXPR, boolean_type_node,
> - gimple_location (outer_cond),
> - outer_cond_code,
> - gimple_cond_lhs (outer_cond),
> - gimple_cond_rhs (outer_cond),
> - gimple_location (inner_cond),
> - inner_cond_code,
> - gimple_cond_lhs (inner_cond),
> - gimple_cond_rhs (inner_cond),
> - single_pred (inner_cond_bb) != outer_cond_bb
> - ? &ts : 0))))
> + && !(flag_tree_ifcombine_fieldmerge
> + && (t = (fold_truth_andor_for_ifcombine
> + (TRUTH_ANDIF_EXPR, boolean_type_node,
> + gimple_location (outer_cond),
> + outer_cond_code,
> + gimple_cond_lhs (outer_cond),
> + gimple_cond_rhs (outer_cond),
> + gimple_location (inner_cond),
> + inner_cond_code,
> + gimple_cond_lhs (inner_cond),
> + gimple_cond_rhs (inner_cond),
> + single_pred (inner_cond_bb) != outer_cond_bb
> + ? &ts : 0)))))
> {
> /* Only combine conditions in this fallback case if the blocks are
> neighbors. */
> @@ -1255,6 +1256,9 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
> /* Record the exit path taken by the outer condition. */
> if (!exit_bb)
> {
> + if (!flag_tree_ifcombine_noncontig)
> + break;
> +
> /* If we have removed the outer condition entirely, we need not
> commit to an exit block yet, it's as if we'd merged the blocks
> and
> were starting afresh. This is sound as long as we never replace
> @@ -1369,6 +1373,15 @@ public:
> {}
>
> /* opt_pass methods: */
> + bool gate (function *) final override {
> + if (!flag_tree_ifcombine)
> + return false;
> +
> + if (flag_tree_ifcombine_noncontig == -1)
> + flag_tree_ifcombine_noncontig = (optimize >= 2 || optimize_size);
> +
> + return flag_tree_ifcombine;
> + }
> unsigned int execute (function *) final override;
>
> }; // class pass_tree_ifcombine
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
