Re: [PATCH] Control all jump threading passes with -fjump-threads.

[Aldy Hernandez <aldyh@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2679 bytes --]



On 9/28/21 9:41 AM, Richard Biener wrote:
> On Tue, Sep 28, 2021 at 8:29 AM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 9/28/2021 12:17 AM, Aldy Hernandez wrote:
>>> On Tue, Sep 28, 2021 at 3:46 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>>>
>>>>
>>>> On 9/27/2021 9:00 AM, Aldy Hernandez wrote:
>>>>> Last year I mentioned that -fthread-jumps was being ignored by the
>>>>> majority of our jump threading passes, and Jeff said he'd be in favor
>>>>> of fixing this.
>>>>>
>>>>> This patch remedies the situation, but it does change existing behavior.
>>>>> Currently -fthread-jumps is only enabled for -O2, -O3, and -Os.  This
>>>>> means that even if we restricted all jump threading passes with
>>>>> -fthread-jumps, DOM jump threading would still seep through since it
>>>>> runs at -O1.
>>>>>
>>>>> I propose this patch, but it does mean that DOM jump threading would
>>>>> have to be explicitly enabled with -O1 -fthread-jumps.  An
>>>>> alternative would be to also offer a specific -fno-dom-threading, but
>>>>> that seems icky.
>>>>>
>>>>> OK pending tests?
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>>         * tree-ssa-threadbackward.c (pass_thread_jumps::gate): Check
>>>>>         flag_thread_jumps.
>>>>>         (pass_early_thread_jumps::gate): Same.
>>>>>         * tree-ssa-threadedge.c (jump_threader::thread_outgoing_edges):
>>>>>         Return if !flag_thread_jumps.
>>>>>         * tree-ssa-threadupdate.c
>>>>>         (jt_path_registry::register_jump_thread): Assert that
>>>>>         flag_thread_jumps is true.
>>>> OK.  Clearly this is going to be even better once we disentangle
>>>> threading from DOM.
>>> Annoyingly, I had to tweak a few more tests, particularly some
>>> -Wuninitialized -O1 ones which seem to depend on DOM jump threading to
>>> give proper diagnostics.  It seems that every change to jump threading
>>> needs tweaks to the Wuninitialized code :-(.
>> Well, a lot of jump threading is there to help eliminate false positives
>> from Wuninitialized by eliminating paths through the CFG that we can
>> prove never execute at runtime.  SO that's not a huge surprise.
> 
> I would have suggested to enable -fthread-jumps at -O1 instead
> and eventually just add && flag_expensive_optimizations to the
> use in cfgcleanup.c to restrict that to -O2+

Hmmm, that's a much better idea.  I was afraid of messing existing 
behavior, but I suppose adding even more false positives for -O1 
-Wuninitialized is worse.

BTW, I plugged one more tweak to the registry in 
remove_jump_threads_including.  No need to go add things to the removed 
edges hash table, if we're not going to thread.

OK pending tests?
Aldy

[-- Attachment #2: p.patch --]
[-- Type: text/x-patch, Size: 9157 bytes --]

commit e0a5b35c8becda7bef37bc6eca1686ab2e762088
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Sep 28 11:33:11 2021 +0200

    Enable jump threading at -O1.
    
    My previous patch gating all jump threading by -fthread-jumps had the
    side effect of turning off DOM jump threading at -O1.  This causes
    numerous -Wuninitialized false positives.  This patch turns on jump
    threading at -O1 to minimize the disruption.
    
    gcc/ChangeLog:
    
            * cfgcleanup.c (pass_jump::execute): Check
            flag_expensive_optimizations.
            (pass_jump_after_combine::gate): Same.
            * doc/invoke.texi (-fthread-jumps): Enable for -O1.
            * opts.c (default_options_table): Enable -fthread-jumps at -O1.
            * tree-ssa-threadupdate.c
            (fwd_jt_path_registry::remove_jump_threads_including): Bail unless
            flag_thread_jumps.
    
    gcc/testsuite/ChangeLog:
    
            * gcc.dg/auto-init-uninit-1.c: Adjust.
            * gcc.dg/auto-init-uninit-15.c: Same.
            * gcc.dg/guality/example.c: Same.
            * gcc.dg/loop-8.c: Same.
            * gcc.dg/strlenopt-40.c: Same.
            * gcc.dg/tree-ssa/pr18133-2.c: Same.
            * gcc.dg/tree-ssa/pr18134.c: Same.
            * gcc.dg/uninit-1.c: Same.
            * gcc.dg/uninit-pr44547.c: Same.
            * gcc.dg/uninit-pr59970.c: Same.

diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
index 7b1e1ba6e80..82fc505ff50 100644
--- a/gcc/cfgcleanup.c
+++ b/gcc/cfgcleanup.c
@@ -3239,7 +3239,8 @@ pass_jump::execute (function *)
   if (dump_file)
     dump_flow_info (dump_file, dump_flags);
   cleanup_cfg ((optimize ? CLEANUP_EXPENSIVE : 0)
-	       | (flag_thread_jumps ? CLEANUP_THREADING : 0));
+	       | (flag_thread_jumps && flag_expensive_optimizations
+		  ? CLEANUP_THREADING : 0));
   return 0;
 }
 
@@ -3274,7 +3275,10 @@ public:
   {}
 
   /* opt_pass methods: */
-  virtual bool gate (function *) { return flag_thread_jumps; }
+  virtual bool gate (function *)
+  {
+    return flag_thread_jumps && flag_expensive_optimizations;
+  }
   virtual unsigned int execute (function *);
 
 }; // class pass_jump_after_combine
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index ba98eab68a5..6d9a107acd0 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10781,7 +10781,7 @@ so, the first branch is redirected to either the destination of the
 second branch or a point immediately following it, depending on whether
 the condition is known to be true or false.
 
-Enabled at levels @option{-O2}, @option{-O3}, @option{-Os}.
+Enabled at levels @option{-O1}, @option{-O2}, @option{-O3}, @option{-Os}.
 
 @item -fsplit-wide-types
 @opindex fsplit-wide-types
diff --git a/gcc/opts.c b/gcc/opts.c
index 38b42db2a4f..6503980cd33 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -569,6 +569,7 @@ static const struct default_options default_options_table[] =
     { OPT_LEVELS_1_PLUS, OPT_freorder_blocks, NULL, 1 },
     { OPT_LEVELS_1_PLUS, OPT_fshrink_wrap, NULL, 1 },
     { OPT_LEVELS_1_PLUS, OPT_fsplit_wide_types, NULL, 1 },
+    { OPT_LEVELS_1_PLUS, OPT_fthread_jumps, NULL, 1 },
     { OPT_LEVELS_1_PLUS, OPT_ftree_builtin_call_dce, NULL, 1 },
     { OPT_LEVELS_1_PLUS, OPT_ftree_ccp, NULL, 1 },
     { OPT_LEVELS_1_PLUS, OPT_ftree_ch, NULL, 1 },
@@ -629,7 +630,6 @@ static const struct default_options default_options_table[] =
 #endif
     { OPT_LEVELS_2_PLUS, OPT_fstrict_aliasing, NULL, 1 },
     { OPT_LEVELS_2_PLUS, OPT_fstore_merging, NULL, 1 },
-    { OPT_LEVELS_2_PLUS, OPT_fthread_jumps, NULL, 1 },
     { OPT_LEVELS_2_PLUS, OPT_ftree_pre, NULL, 1 },
     { OPT_LEVELS_2_PLUS, OPT_ftree_switch_conversion, NULL, 1 },
     { OPT_LEVELS_2_PLUS, OPT_ftree_tail_merge, NULL, 1 },
diff --git a/gcc/testsuite/gcc.dg/auto-init-uninit-1.c b/gcc/testsuite/gcc.dg/auto-init-uninit-1.c
index ce8909623ab..502db591222 100644
--- a/gcc/testsuite/gcc.dg/auto-init-uninit-1.c
+++ b/gcc/testsuite/gcc.dg/auto-init-uninit-1.c
@@ -1,5 +1,5 @@
 /* Spurious uninitialized variable warnings, case 1.
    Taken from cppfiles.c (merge_include_chains) */
 /* { dg-do compile } */
-/* { dg-options "-O -Wuninitialized -fthread-jumps -ftrivial-auto-var-init=zero" } */
+/* { dg-options "-O -Wuninitialized -ftrivial-auto-var-init=zero" } */
 #include "uninit-1.c"
diff --git a/gcc/testsuite/gcc.dg/auto-init-uninit-15.c b/gcc/testsuite/gcc.dg/auto-init-uninit-15.c
index b8f6e2b57d5..121f0cff274 100644
--- a/gcc/testsuite/gcc.dg/auto-init-uninit-15.c
+++ b/gcc/testsuite/gcc.dg/auto-init-uninit-15.c
@@ -5,7 +5,7 @@
    But it is of course ok if we warn in bar about uninitialized use
    of j.  (not xfailed alternative)  */
 /* { dg-do compile } */
-/* { dg-options "-O1 -Wuninitialized -fthread-jumps -ftrivial-auto-var-init=zero" } */
+/* { dg-options "-O1 -Wuninitialized -ftrivial-auto-var-init=zero" } */
 
 inline int
 foo (int i)
diff --git a/gcc/testsuite/gcc.dg/guality/example.c b/gcc/testsuite/gcc.dg/guality/example.c
index 37564e55cd1..32014e2b4c0 100644
--- a/gcc/testsuite/gcc.dg/guality/example.c
+++ b/gcc/testsuite/gcc.dg/guality/example.c
@@ -1,4 +1,5 @@
 /* { dg-options "-g" } */
+/* { dg-do run { xfail { ! aarch64*-*-* } } } */
 /* { dg-xfail-run-if "" aarch64*-*-* "*" { "-O[01g]" } } */
 
 #define GUALITY_DONT_FORCE_LIVE_AFTER -1
diff --git a/gcc/testsuite/gcc.dg/loop-8.c b/gcc/testsuite/gcc.dg/loop-8.c
index e5218eb4053..90ea1c45524 100644
--- a/gcc/testsuite/gcc.dg/loop-8.c
+++ b/gcc/testsuite/gcc.dg/loop-8.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O1 -fthread-jumps -fdump-rtl-loop2_invariant" } */
+/* { dg-options "-O1 -fdump-rtl-loop2_invariant" } */
 /* { dg-skip-if "unexpected IV" { "hppa*-*-* mips*-*-* visium-*-* powerpc*-*-* riscv*-*-* mmix-*-* vax-*-*" } } */
 /* Load immediate on condition is available from z13 on and prevents moving
    the load out of the loop, so always run this test with -march=zEC12 that
diff --git a/gcc/testsuite/gcc.dg/strlenopt-40.c b/gcc/testsuite/gcc.dg/strlenopt-40.c
index 7b799104708..7a97ebb8fe5 100644
--- a/gcc/testsuite/gcc.dg/strlenopt-40.c
+++ b/gcc/testsuite/gcc.dg/strlenopt-40.c
@@ -1,7 +1,7 @@
 /* PR tree-optimization/83671 - fix for false positive reported by
    -Wstringop-overflow does not work with inlining
    { dg-do compile }
-   { dg-options "-O1 -fthread-jumps -fdump-tree-optimized" } */
+   { dg-options "-O1 -fdump-tree-optimized" } */
 
 #include "strlenopt.h"
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr18133-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr18133-2.c
index 1b409852189..8717640e327 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr18133-2.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr18133-2.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O1 -fthread-jumps -fdump-tree-optimized-blocks" } */
+/* { dg-options "-O1 -fdump-tree-optimized-blocks" } */
 
 int c, d;
 
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr18134.c b/gcc/testsuite/gcc.dg/tree-ssa/pr18134.c
index d7f5d241eb9..cd40ab2c162 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr18134.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr18134.c
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-options "-O1 -fthread-jumps -fdump-tree-optimized" } */
+/* { dg-options "-O1 -fdump-tree-optimized" } */
 
 int  foo (int a)
 {
diff --git a/gcc/testsuite/gcc.dg/uninit-1.c b/gcc/testsuite/gcc.dg/uninit-1.c
index 156d34ff732..060ec250ba7 100644
--- a/gcc/testsuite/gcc.dg/uninit-1.c
+++ b/gcc/testsuite/gcc.dg/uninit-1.c
@@ -1,7 +1,7 @@
 /* Spurious uninitialized variable warnings, case 1.
    Taken from cppfiles.c (merge_include_chains) */
 /* { dg-do compile } */
-/* { dg-options "-O -Wuninitialized -fthread-jumps" } */
+/* { dg-options "-O -Wuninitialized" } */
 
 struct list
 {
diff --git a/gcc/testsuite/gcc.dg/uninit-pr44547.c b/gcc/testsuite/gcc.dg/uninit-pr44547.c
index f1c3b034d14..ee1035ad7b8 100644
--- a/gcc/testsuite/gcc.dg/uninit-pr44547.c
+++ b/gcc/testsuite/gcc.dg/uninit-pr44547.c
@@ -1,7 +1,7 @@
 /* PR tree-optimization/44547 - -Wuninitialized reports false warning
    in nested switch statements
    { dg-do compile }
-   { dg-options "-O1 -Wall -fthread-jumps" } */
+   { dg-options "-O1 -Wall" } */
 
 __attribute__ ((noipa)) int test_O1 (int argc)
 {
diff --git a/gcc/testsuite/gcc.dg/uninit-pr59970.c b/gcc/testsuite/gcc.dg/uninit-pr59970.c
index d0c41b8480d..145af657a76 100644
--- a/gcc/testsuite/gcc.dg/uninit-pr59970.c
+++ b/gcc/testsuite/gcc.dg/uninit-pr59970.c
@@ -41,7 +41,7 @@ d_demangle_callback_O1 (const char *mangled)
 #pragma GCC pop_options
 
 
-#pragma GCC optimize ("Og,thread-jumps")
+#pragma GCC optimize ("Og")
 
 __attribute__ ((noipa)) int
 d_demangle_callback_Og (const char *mangled)
diff --git a/gcc/tree-ssa-threadupdate.c b/gcc/tree-ssa-threadupdate.c
index 905dea2e6ca..dcabfdb30d2 100644
--- a/gcc/tree-ssa-threadupdate.c
+++ b/gcc/tree-ssa-threadupdate.c
@@ -2575,7 +2575,7 @@ valid_jump_thread_path (vec<jump_thread_edge *> *path)
 void
 fwd_jt_path_registry::remove_jump_threads_including (edge_def *e)
 {
-  if (!m_paths.exists ())
+  if (!m_paths.exists () || !flag_thread_jumps)
     return;
 
   edge *slot = m_removed_edges->find_slot (e, INSERT);