From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id D7F33385F024 for ; Thu, 2 Apr 2020 09:12:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D7F33385F024 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rguenther@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 42B53AEC7; Thu, 2 Apr 2020 09:12:49 +0000 (UTC) Date: Thu, 2 Apr 2020 11:12:48 +0200 (CEST) From: Richard Biener To: Jason Merrill cc: gcc-patches@gcc.gnu.org, "Joseph S. Myers" Subject: Re: [PATCH][RFC] c/94392 - only enable -ffinite-loops for C++ In-Reply-To: <0f331251-85c6-2cfe-e7aa-54bc7966ce57@redhat.com> Message-ID: References: <0f331251-85c6-2cfe-e7aa-54bc7966ce57@redhat.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 X-Spam-Status: No, score=-29.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Apr 2020 09:12:52 -0000 On Wed, 1 Apr 2020, Jason Merrill wrote: > On 4/1/20 9:36 AM, Richard Biener wrote: > > This does away with enabling -ffinite-loops at -O2+ for all languages > > and instead enables it selectively for C++ only. > > > > Jason, I didn't find a reference as to when the forward progress > > guarantee was introduced to C++ so I randomly chose C++11, is that > > correct? > > C++11 says "Implementations should ensure that all unblocked threads > eventually make progress." > > C++17 adds the "Forward progress" section that says > > "The implementation may assume that any thread will eventually do one of the > following: > — terminate, > — make a call to a library I/O function, > — perform an access through a volatile glvalue, or > — perform a synchronization operation or an atomic operation. > [Note: This is intended to allow compiler transformations such as removal of > empty loops, even when termination cannot be proven. — end note]" OK, so I assume using C++11 as in the patch is fine. I'm retesting the following currently which factors in Honzas comments turning -ffinite-loops into a per-loop setting at CFG construction time (the same place where we'd handle #pragma induced ANNOTATE_EXPRs for such feature - for GCC11 we might implement the C side this way which has more restrictive wording where such forward progress can be assumed) Thanks, Richard. [PATCH] c/94392 - only enable -ffinite-loops for C++ This does away with enabling -ffinite-loops at -O2+ for all languages and instead enables it selectively for C++ only. It also makes -ffinite-loops loop-private at CFG construction time fixing correctness issues with inlining. 2020-04-02 Richard Biener PR c/94392 * c-opts.c (c_common_post_options): Enable -ffinite-loops for -O2 and C++11 or newer. * common.opt (ffinite-loops): Initialize to zero. * opts.c (default_options_table): Remove OPT_ffinite_loops entry. * cfgloop.h (loop::finite_p): New member. * cfgloopmanip.c (copy_loop_info): Copy finite_p. * ipa-icf-gimple.c (func_checker::compare_loops): Compare finite_p. * lto-streamer-in.c (input_cfg): Stream finite_p. * lto-streamer-out.c (output_cfg): Likewise. * tree-cfg.c (replace_loop_annotate): Initialize finite_p from flag_finite_loops at CFG build time. * tree-ssa-loop-niter.c (finite_loop_p): Check the loops finite_p flag instead of flag_finite_loops. * doc/invoke.texi (ffinite-loops): Adjust documentation of default setting. * gcc.dg/torture/pr94392.c: New testcase. --- gcc/c-family/c-opts.c | 4 ++++ gcc/cfgloop.h | 4 ++++ gcc/cfgloopmanip.c | 1 + gcc/common.opt | 2 +- gcc/doc/invoke.texi | 3 ++- gcc/ipa-icf-gimple.c | 2 ++ gcc/lto-streamer-in.c | 1 + gcc/lto-streamer-out.c | 1 + gcc/opts.c | 1 - gcc/testsuite/gcc.dg/torture/pr94392.c | 22 ++++++++++++++++++++++ gcc/tree-cfg.c | 3 +++ gcc/tree-ssa-loop-niter.c | 2 +- 12 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/torture/pr94392.c diff --git a/gcc/c-family/c-opts.c b/gcc/c-family/c-opts.c index 6b6c754ad86..58ba0948e79 100644 --- a/gcc/c-family/c-opts.c +++ b/gcc/c-family/c-opts.c @@ -989,6 +989,10 @@ c_common_post_options (const char **pfilename) SET_OPTION_IF_UNSET (&global_options, &global_options_set, flag_new_ttp, cxx_dialect >= cxx17); + /* C++11 guarantees forward progress. */ + SET_OPTION_IF_UNSET (&global_options, &global_options_set, flag_finite_loops, + optimize >= 2 && cxx_dialect >= cxx11); + if (cxx_dialect >= cxx11) { /* If we're allowing C++0x constructs, don't warn about C++98 diff --git a/gcc/cfgloop.h b/gcc/cfgloop.h index 1c49a8b8c2d..18b404e292f 100644 --- a/gcc/cfgloop.h +++ b/gcc/cfgloop.h @@ -226,6 +226,10 @@ public: /* True if the loop is part of an oacc kernels region. */ unsigned in_oacc_kernels_region : 1; + /* True if the loop is known to be finite. This is a localized + flag_finite_loops or similar pragmas state. */ + unsigned finite_p : 1; + /* The number of times to unroll the loop. 0 means no information given, just do what we always do. A value of 1 means do not unroll the loop. A value of USHRT_MAX means unroll with no specific unrolling factor. diff --git a/gcc/cfgloopmanip.c b/gcc/cfgloopmanip.c index c9375565f62..50c7267ec49 100644 --- a/gcc/cfgloopmanip.c +++ b/gcc/cfgloopmanip.c @@ -1023,6 +1023,7 @@ copy_loop_info (class loop *loop, class loop *target) target->dont_vectorize = loop->dont_vectorize; target->force_vectorize = loop->force_vectorize; target->in_oacc_kernels_region = loop->in_oacc_kernels_region; + target->finite_p = loop->finite_p; target->unroll = loop->unroll; target->owned_clique = loop->owned_clique; } diff --git a/gcc/common.opt b/gcc/common.opt index 4368910cb54..bb2ea4c905d 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -1490,7 +1490,7 @@ Common Report Var(flag_finite_math_only) Optimization SetByCombined Assume no NaNs or infinities are generated. ffinite-loops -Common Report Var(flag_finite_loops) Optimization +Common Report Var(flag_finite_loops) Optimization Init(0) Assume that loops with an exit will terminate and not loop indefinitely. ffixed- diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 412750c1fc9..142bfeacead 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -10432,7 +10432,8 @@ Assume that a loop with an exit will eventually take the exit and not loop indefinitely. This allows the compiler to remove loops that otherwise have no side-effects, not considering eventual endless looping as such. -This option is enabled by default at @option{-O2}. +This option is enabled by default at @option{-O2} for C++ with -std=c++11 +or higher. @item -ftree-dominator-opts @opindex ftree-dominator-opts diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c index 3e5b2d4bd6d..d306fec56ce 100644 --- a/gcc/ipa-icf-gimple.c +++ b/gcc/ipa-icf-gimple.c @@ -395,6 +395,8 @@ func_checker::compare_loops (basic_block bb1, basic_block bb2) return return_false_with_msg ("dont_vectorize"); if (l1->force_vectorize != l2->force_vectorize) return return_false_with_msg ("force_vectorize"); + if (l1->finite_p != l2->finite_p) + return return_false_with_msg ("finite_p"); if (l1->unroll != l2->unroll) return return_false_with_msg ("unroll"); if (!compare_variable_decl (l1->simduid, l2->simduid)) diff --git a/gcc/lto-streamer-in.c b/gcc/lto-streamer-in.c index 9566e5ee102..244f5b8aa5c 100644 --- a/gcc/lto-streamer-in.c +++ b/gcc/lto-streamer-in.c @@ -821,6 +821,7 @@ input_cfg (class lto_input_block *ib, class data_in *data_in, loop->owned_clique = streamer_read_hwi (ib); loop->dont_vectorize = streamer_read_hwi (ib); loop->force_vectorize = streamer_read_hwi (ib); + loop->finite_p = streamer_read_hwi (ib); loop->simduid = stream_read_tree (ib, data_in); place_new_loop (fn, loop); diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c index a219c1d0dd1..52ef94718db 100644 --- a/gcc/lto-streamer-out.c +++ b/gcc/lto-streamer-out.c @@ -1950,6 +1950,7 @@ output_cfg (struct output_block *ob, struct function *fn) streamer_write_hwi (ob, loop->owned_clique); streamer_write_hwi (ob, loop->dont_vectorize); streamer_write_hwi (ob, loop->force_vectorize); + streamer_write_hwi (ob, loop->finite_p); stream_write_tree (ob, loop->simduid, true); } diff --git a/gcc/opts.c b/gcc/opts.c index 5dc7d65dedd..d4df8627bf7 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -478,7 +478,6 @@ static const struct default_options default_options_table[] = { OPT_LEVELS_2_PLUS, OPT_fdevirtualize, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fdevirtualize_speculatively, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fexpensive_optimizations, NULL, 1 }, - { OPT_LEVELS_2_PLUS, OPT_ffinite_loops, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fgcse, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_fhoist_adjacent_loads, NULL, 1 }, { OPT_LEVELS_2_PLUS, OPT_findirect_inlining, NULL, 1 }, diff --git a/gcc/testsuite/gcc.dg/torture/pr94392.c b/gcc/testsuite/gcc.dg/torture/pr94392.c new file mode 100644 index 00000000000..373f18ce983 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr94392.c @@ -0,0 +1,22 @@ +/* { dg-do compile } */ +/* { dg-skip-if "finite loops" { *-*-* } { "-ffinite-loops" } } */ +/* { dg-skip-if "LTO optimizes the test" { *-*-* } { "-flto" } } */ +/* { dg-additional-options "-fdump-tree-optimized" } */ + +int a, b; + +int +main() +{ + while (1) + { + /* Try really hard. */ + if (a != b) + return 1; + } + return 0; +} + +/* ISO C does not guarantee forward progress like C++ does so we + cannot assume the loop is finite and optimize it to return 1. */ +/* { dg-final { scan-tree-dump "if" "optimized" } } */ diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index f7b817d94e6..e99fb9ff5d1 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -324,6 +324,9 @@ replace_loop_annotate (void) /* Then look into the latch, if any. */ if (loop->latch) replace_loop_annotate_in_block (loop->latch, loop); + + /* Push the global flag_finite_loops state down to individual loops. */ + loop->finite_p = flag_finite_loops; } /* Remove IFN_ANNOTATE. Safeguard for the case loop->latch == NULL. */ diff --git a/gcc/tree-ssa-loop-niter.c b/gcc/tree-ssa-loop-niter.c index 6e6df0bfdb8..7d61ef080eb 100644 --- a/gcc/tree-ssa-loop-niter.c +++ b/gcc/tree-ssa-loop-niter.c @@ -2834,7 +2834,7 @@ finite_loop_p (class loop *loop) return true; } - if (flag_finite_loops) + if (loop->finite_p) { unsigned i; vec exits = get_loop_exit_edges (loop); -- 2.13.7