ifcombine: avoid unsound forwarder-enabled combinations [PR117723]

Message ID orr06ucvyh.fsf@lxoliva.fsfla.org
State Committed
Commit 70f7c603da67adff13e41831ab8ec4e2f6fadc2c
Headers
Series ifcombine: avoid unsound forwarder-enabled combinations [PR117723] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_check--master-arm fail Patch failed to apply

Commit Message

Alexandre Oliva Nov. 29, 2024, 7:58 a.m. UTC
  When ifcombining contiguous blocks, we can follow forwarder blocks and
reverse conditions to enable combinations, but when there are
intervening blocks, we have to constrain ourselves to paths to the
exit that share the PHI args with all intervening blocks.

Avoiding considering forwarders when intervening blocks were present
would match the preexisting test, but we can do better, recording in
case a forwarded path corresponds to the outer block's exit path, and
insisting on not combining through any other path but the one that was
verified as corresponding.  The latter is what this patch implements.

While at that, I've fixed some typos, introduced early testing before
computing the exit path to avoid it when computing it would be
wasteful, or when avoiding it can enable other sound combinations.

Regstrapped on x86_64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	PR tree-optimization/117723
	* tree-ssa-ifcombine.cc (tree_ssa_ifcombine_bb): Record
	forwarder blocks in path to exit, and stick to them.  Avoid
	computing the exit if obviously not needed, and if that
	enables additional optimizations.
	(tree_ssa_ifcombine_bb_1): Fix typos.

for  gcc/testsuite/ChangeLog

	PR tree-optimization/117723
	* gcc.dg/torture/ifcmb-1.c: New.
---
 gcc/testsuite/gcc.dg/torture/ifcmb-1.c |   63 +++++++++++++++++
 gcc/tree-ssa-ifcombine.cc              |  116 +++++++++++++++++++++++++++-----
 2 files changed, 161 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/ifcmb-1.c
  

Comments

Richard Biener Nov. 29, 2024, 8:17 a.m. UTC | #1
On Fri, Nov 29, 2024 at 8:59 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
>
> When ifcombining contiguous blocks, we can follow forwarder blocks and
> reverse conditions to enable combinations, but when there are
> intervening blocks, we have to constrain ourselves to paths to the
> exit that share the PHI args with all intervening blocks.
>
> Avoiding considering forwarders when intervening blocks were present
> would match the preexisting test, but we can do better, recording in
> case a forwarded path corresponds to the outer block's exit path, and
> insisting on not combining through any other path but the one that was
> verified as corresponding.  The latter is what this patch implements.
>
> While at that, I've fixed some typos, introduced early testing before
> computing the exit path to avoid it when computing it would be
> wasteful, or when avoiding it can enable other sound combinations.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?

OK.

Thanks,
Richard.

>
> for  gcc/ChangeLog
>
>         PR tree-optimization/117723
>         * tree-ssa-ifcombine.cc (tree_ssa_ifcombine_bb): Record
>         forwarder blocks in path to exit, and stick to them.  Avoid
>         computing the exit if obviously not needed, and if that
>         enables additional optimizations.
>         (tree_ssa_ifcombine_bb_1): Fix typos.
>
> for  gcc/testsuite/ChangeLog
>
>         PR tree-optimization/117723
>         * gcc.dg/torture/ifcmb-1.c: New.
> ---
>  gcc/testsuite/gcc.dg/torture/ifcmb-1.c |   63 +++++++++++++++++
>  gcc/tree-ssa-ifcombine.cc              |  116 +++++++++++++++++++++++++++-----
>  2 files changed, 161 insertions(+), 18 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/ifcmb-1.c
>
> diff --git a/gcc/testsuite/gcc.dg/torture/ifcmb-1.c b/gcc/testsuite/gcc.dg/torture/ifcmb-1.c
> new file mode 100644
> index 0000000000000..2431a548598fc
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/ifcmb-1.c
> @@ -0,0 +1,63 @@
> +/* { dg-do run } */
> +
> +/* Test that we do NOT perform unsound transformations for any of these cases.
> +   Forwarding blocks to the exit block used to enable some of them.  */
> +
> +[[gnu::noinline]]
> +int f0 (int a, int b) {
> +  if ((a & 1))
> +    return 0;
> +  if (b)
> +    return 1;
> +  if (!(a & 2))
> +    return 0;
> +  else
> +    return 1;
> +}
> +
> +[[gnu::noinline]]
> +int f1 (int a, int b) {
> +  if (!(a & 1))
> +    return 0;
> +  if (b)
> +    return 1;
> +  if ((a & 2))
> +    return 1;
> +  else
> +    return 0;
> +}
> +
> +[[gnu::noinline]]
> +int f2 (int a, int b) {
> +  if ((a & 1))
> +    return 0;
> +  if (b)
> +    return 1;
> +  if (!(a & 2))
> +    return 0;
> +  else
> +    return 1;
> +}
> +
> +[[gnu::noinline]]
> +int f3 (int a, int b) {
> +  if (!(a & 1))
> +    return 0;
> +  if (b)
> +    return 1;
> +  if ((a & 2))
> +    return 1;
> +  else
> +    return 0;
> +}
> +
> +int main() {
> +  if (f0 (0, 1) != 1)
> +    __builtin_abort();
> +  if (f1 (1, 1) != 1)
> +    __builtin_abort();
> +  if (f2 (2, 1) != 1)
> +    __builtin_abort();
> +  if (f3 (3, 1) != 1)
> +    __builtin_abort();
> +}
> diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> index e389b12aa37db..a87bf1210776f 100644
> --- a/gcc/tree-ssa-ifcombine.cc
> +++ b/gcc/tree-ssa-ifcombine.cc
> @@ -1077,7 +1077,7 @@ tree_ssa_ifcombine_bb_1 (basic_block inner_cond_bb, basic_block outer_cond_bb,
>      }
>
>    /* The || form is characterized by a common then_bb with the
> -     two edges leading to it mergable.  The latter is guaranteed
> +     two edges leading to it mergeable.  The latter is guaranteed
>       by matching PHI arguments in the then_bb and the inner cond_bb
>       having no side-effects.  */
>    if (phi_pred_bb != then_bb
> @@ -1088,7 +1088,7 @@ tree_ssa_ifcombine_bb_1 (basic_block inner_cond_bb, basic_block outer_cond_bb,
>            <outer_cond_bb>
>              if (q) goto then_bb; else goto inner_cond_bb;
>            <inner_cond_bb>
> -            if (q) goto then_bb; else goto ...;
> +            if (p) goto then_bb; else goto ...;
>            <then_bb>
>              ...
>         */
> @@ -1104,7 +1104,7 @@ tree_ssa_ifcombine_bb_1 (basic_block inner_cond_bb, basic_block outer_cond_bb,
>            <outer_cond_bb>
>              if (q) goto inner_cond_bb; else goto then_bb;
>            <inner_cond_bb>
> -            if (q) goto then_bb; else goto ...;
> +            if (p) goto then_bb; else goto ...;
>            <then_bb>
>              ...
>         */
> @@ -1139,13 +1139,18 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
>       Look for an OUTER_COND_BBs to combine with INNER_COND_BB.  They need not
>       be contiguous, as long as inner and intervening blocks have no side
>       effects, and are either single-entry-single-exit or conditionals choosing
> -     between the same EXIT_BB with the same PHI args, and the path leading to
> -     INNER_COND_BB.  ??? We could potentially handle multi-block
> -     single-entry-single-exit regions, but the loop below only deals with
> -     single-entry-single-exit individual intervening blocks.  Larger regions
> -     without side effects are presumably rare, so it's probably not worth the
> -     effort.  */
> -  for (basic_block bb = inner_cond_bb, outer_cond_bb, exit_bb = NULL;
> +     between the same EXIT_BB with the same PHI args, possibly through an
> +     EXIT_PRED, and the path leading to INNER_COND_BB.  EXIT_PRED will be set
> +     just before (along with a successful combination) or just after setting
> +     EXIT_BB, to either THEN_BB, ELSE_BB, or INNER_COND_BB.  ??? We could
> +     potentially handle multi-block single-entry-single-exit regions, but the
> +     loop below only deals with single-entry-single-exit individual intervening
> +     blocks.  Larger regions without side effects are presumably rare, so it's
> +     probably not worth the effort.  */
> +  for (basic_block bb = inner_cond_bb, outer_cond_bb, exit_bb = NULL,
> +        /* This initialization shouldn't be needed, but in case the compiler
> +           is not smart enough to tell, make it harmless.  */
> +        exit_pred = NULL;
>         single_pred_p (bb) && bb_no_side_effects_p (bb);
>         bb = outer_cond_bb)
>      {
> @@ -1186,10 +1191,13 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
>          checking of same phi args.  */
>        if (known_succ_p (outer_cond_bb))
>         changed = false;
> -      else if (tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb,
> -                                       then_bb, else_bb, inner_cond_bb, bb))
> -       changed = true;
> -      else if (forwarder_block_to (else_bb, then_bb))
> +      else if ((!exit_bb || exit_pred == inner_cond_bb)
> +              && tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb,
> +                                          then_bb, else_bb, inner_cond_bb, bb))
> +       changed = true, exit_pred = inner_cond_bb;
> +      else if (exit_bb
> +              ? exit_pred == else_bb
> +              : forwarder_block_to (else_bb, then_bb))
>         {
>           /* Other possibilities for the && form, if else_bb is
>              empty forwarder block to then_bb.  Compared to the above simpler
> @@ -1199,9 +1207,11 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
>              edge from outer_cond_bb and the forwarder block.  */
>           if (tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb, else_bb,
>                                        then_bb, else_bb, bb))
> -           changed = true;
> +           changed = true, exit_pred = else_bb;
>         }
> -      else if (forwarder_block_to (then_bb, else_bb))
> +      else if (exit_bb
> +              ? exit_pred == then_bb
> +              : forwarder_block_to (then_bb, else_bb))
>         {
>           /* Other possibilities for the || form, if then_bb is
>              empty forwarder block to else_bb.  Compared to the above simpler
> @@ -1211,7 +1221,7 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
>              edge from outer_cond_bb and the forwarder block.  */
>           if (tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb, else_bb,
>                                        then_bb, then_bb, bb))
> -           changed = true;
> +           changed = true, exit_pred = then_bb;
>         }
>
>        if (changed)
> @@ -1222,15 +1232,85 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
>        if (changed && known_succ_p (inner_cond_bb))
>         break;
>
> +      /* Starting at this point in the loop, we start preparing to attempt
> +        combinations in which OUTER_COND_BB will be an intervening block.
> +        Checking that it has a single predecessor is a very cheap test, unlike
> +        the PHI args tests below, so test it early and hopefully save the more
> +        expensive tests in case we won't be able to try other blocks.  */
> +      if (!single_pred_p (outer_cond_bb))
> +       break;
> +
>        /* Record the exit path taken by the outer condition.  */
>        if (!exit_bb)
>         {
> +         /* 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
> +            the outer condition with a constant that leads away from the inner
> +            block.  Here's why we never do: when combining contiguous
> +            conditions, we replace the inner cond, and replace the outer cond
> +            with a constant that leads to inner, so this case is good.  When
> +            combining noncontiguous blocks, we normally modify outer, and
> +            replace inner with a constant or remainders of the original
> +            condition that couldn't be combined.  This test would normally not
> +            hit with noncontiguous blocks, because we'd have computed EXIT_BB
> +            before reaching the noncontiguous outer block.  However, if all
> +            intervening blocks are unconditional, including those just made
> +            unconditional, we may replace outer instead of inner with the
> +            combined condition.  If the combined noncontiguous conditions are
> +            mutually exclusive, we could end up with a constant outer
> +            condition, but then, the inner condition would also be a constant,
> +            and then we'd stop iterating because of the known_succ_p
> +            (inner_cond_bb) test above.  */
> +         if (changed && known_succ_p (outer_cond_bb))
> +           continue;
> +
>           if (recognize_if_then_else (outer_cond_bb, &then_bb, &bb, true))
>             exit_bb = then_bb;
>           else if (recognize_if_then_else (outer_cond_bb, &bb, &else_bb, true))
>             exit_bb = else_bb;
>           else
>             break;
> +
> +         /* Find out which path from INNER_COND_BB shares PHI args with the
> +            edge (OUTER_COND_BB->EXIT_BB).  That path may involve a forwarder
> +            block, whether THEN_BB or ELSE_BB, and we need to know which one
> +            satisfies the condition to avoid combinations that could use
> +            different forwarding arrangements, because they would be unsound.
> +            E.g., given (a ? 0 : b ? 1 : c ? 1 : 0), after trying to merge b
> +            and c, we test that both share the same exit block, with the same
> +            value 1.  Whether or not that involves a forwarder block, if we
> +            don't go through the same (possibly absent) forwarder block in
> +            subsequent attempted combinations, e.g. a with c, we could find
> +            that a and inverted c share the same exit block with a different
> +            value, namely 0, which would enable an unsound merge.  We need all
> +            of inner, intervening and outer blocks to reach the same exit with
> +            the same value for the transformation to be sound.  So here we
> +            determine how to get to EXIT_BB from outer and inner with the same
> +            PHI values, record that in EXIT_PRED, and then subsequent
> +            combination attempts that have OUTER_COND_BB as an intervening
> +            block will ensure the same path to exit is taken, skipping unsound
> +            transformations.  */
> +         if (changed)
> +           /* EXIT_PRED was set along with CHANGED, and the successful
> +              combination already checked for the same PHI args.  */;
> +         else if (same_phi_args_p (outer_cond_bb, inner_cond_bb, exit_bb))
> +           exit_pred = inner_cond_bb;
> +         else if (then_bb == exit_bb
> +                  && forwarder_block_to (else_bb, then_bb)
> +                  && same_phi_args_p (outer_cond_bb, else_bb, exit_bb))
> +           exit_pred = else_bb;
> +         else if (else_bb == exit_bb
> +                  && forwarder_block_to (then_bb, else_bb)
> +                  && same_phi_args_p (outer_cond_bb, then_bb, exit_bb))
> +           exit_pred = then_bb;
> +         else
> +           /* If none of the paths share the same PHI args, no combination is
> +              viable.  */
> +           break;
> +         /* Skip the PHI args test below, it's redundant with the tests we've
> +            just performed.  */
> +         continue;
>         }
>
>        /* Before trying an earlier block, make sure INNER_COND_BB and the
> @@ -1245,7 +1325,7 @@ tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
>          of outer, we'd get ([1 ? 0 :] b ? 1 : (a | c) ? 0 : -1), which would
>          yield 1 rather than 0 when (a).  */
>        if (!changed
> -         && !same_phi_args_p (outer_cond_bb, inner_cond_bb, exit_bb))
> +         && !same_phi_args_p (outer_cond_bb, exit_pred, exit_bb))
>         break;
>      }
>
>
>
> --
> 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
  

Patch

diff --git a/gcc/testsuite/gcc.dg/torture/ifcmb-1.c b/gcc/testsuite/gcc.dg/torture/ifcmb-1.c
new file mode 100644
index 0000000000000..2431a548598fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/ifcmb-1.c
@@ -0,0 +1,63 @@ 
+/* { dg-do run } */
+
+/* Test that we do NOT perform unsound transformations for any of these cases.
+   Forwarding blocks to the exit block used to enable some of them.  */
+
+[[gnu::noinline]]
+int f0 (int a, int b) {
+  if ((a & 1))
+    return 0;
+  if (b)
+    return 1;
+  if (!(a & 2))
+    return 0;
+  else
+    return 1;
+}
+
+[[gnu::noinline]]
+int f1 (int a, int b) {
+  if (!(a & 1))
+    return 0;
+  if (b)
+    return 1;
+  if ((a & 2))
+    return 1;
+  else
+    return 0;
+}
+
+[[gnu::noinline]]
+int f2 (int a, int b) {
+  if ((a & 1))
+    return 0;
+  if (b)
+    return 1;
+  if (!(a & 2))
+    return 0;
+  else
+    return 1;
+}
+
+[[gnu::noinline]]
+int f3 (int a, int b) {
+  if (!(a & 1))
+    return 0;
+  if (b)
+    return 1;
+  if ((a & 2))
+    return 1;
+  else
+    return 0;
+}
+
+int main() {
+  if (f0 (0, 1) != 1)
+    __builtin_abort();
+  if (f1 (1, 1) != 1)
+    __builtin_abort();
+  if (f2 (2, 1) != 1)
+    __builtin_abort();
+  if (f3 (3, 1) != 1)
+    __builtin_abort();
+}
diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
index e389b12aa37db..a87bf1210776f 100644
--- a/gcc/tree-ssa-ifcombine.cc
+++ b/gcc/tree-ssa-ifcombine.cc
@@ -1077,7 +1077,7 @@  tree_ssa_ifcombine_bb_1 (basic_block inner_cond_bb, basic_block outer_cond_bb,
     }
 
   /* The || form is characterized by a common then_bb with the
-     two edges leading to it mergable.  The latter is guaranteed
+     two edges leading to it mergeable.  The latter is guaranteed
      by matching PHI arguments in the then_bb and the inner cond_bb
      having no side-effects.  */
   if (phi_pred_bb != then_bb
@@ -1088,7 +1088,7 @@  tree_ssa_ifcombine_bb_1 (basic_block inner_cond_bb, basic_block outer_cond_bb,
 	   <outer_cond_bb>
 	     if (q) goto then_bb; else goto inner_cond_bb;
 	   <inner_cond_bb>
-	     if (q) goto then_bb; else goto ...;
+	     if (p) goto then_bb; else goto ...;
 	   <then_bb>
 	     ...
        */
@@ -1104,7 +1104,7 @@  tree_ssa_ifcombine_bb_1 (basic_block inner_cond_bb, basic_block outer_cond_bb,
 	   <outer_cond_bb>
 	     if (q) goto inner_cond_bb; else goto then_bb;
 	   <inner_cond_bb>
-	     if (q) goto then_bb; else goto ...;
+	     if (p) goto then_bb; else goto ...;
 	   <then_bb>
 	     ...
        */
@@ -1139,13 +1139,18 @@  tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
      Look for an OUTER_COND_BBs to combine with INNER_COND_BB.  They need not
      be contiguous, as long as inner and intervening blocks have no side
      effects, and are either single-entry-single-exit or conditionals choosing
-     between the same EXIT_BB with the same PHI args, and the path leading to
-     INNER_COND_BB.  ??? We could potentially handle multi-block
-     single-entry-single-exit regions, but the loop below only deals with
-     single-entry-single-exit individual intervening blocks.  Larger regions
-     without side effects are presumably rare, so it's probably not worth the
-     effort.  */
-  for (basic_block bb = inner_cond_bb, outer_cond_bb, exit_bb = NULL;
+     between the same EXIT_BB with the same PHI args, possibly through an
+     EXIT_PRED, and the path leading to INNER_COND_BB.  EXIT_PRED will be set
+     just before (along with a successful combination) or just after setting
+     EXIT_BB, to either THEN_BB, ELSE_BB, or INNER_COND_BB.  ??? We could
+     potentially handle multi-block single-entry-single-exit regions, but the
+     loop below only deals with single-entry-single-exit individual intervening
+     blocks.  Larger regions without side effects are presumably rare, so it's
+     probably not worth the effort.  */
+  for (basic_block bb = inner_cond_bb, outer_cond_bb, exit_bb = NULL,
+	 /* This initialization shouldn't be needed, but in case the compiler
+	    is not smart enough to tell, make it harmless.  */
+	 exit_pred = NULL;
        single_pred_p (bb) && bb_no_side_effects_p (bb);
        bb = outer_cond_bb)
     {
@@ -1186,10 +1191,13 @@  tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
 	 checking of same phi args.  */
       if (known_succ_p (outer_cond_bb))
 	changed = false;
-      else if (tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb,
-					then_bb, else_bb, inner_cond_bb, bb))
-	changed = true;
-      else if (forwarder_block_to (else_bb, then_bb))
+      else if ((!exit_bb || exit_pred == inner_cond_bb)
+	       && tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb,
+					   then_bb, else_bb, inner_cond_bb, bb))
+	changed = true, exit_pred = inner_cond_bb;
+      else if (exit_bb
+	       ? exit_pred == else_bb
+	       : forwarder_block_to (else_bb, then_bb))
 	{
 	  /* Other possibilities for the && form, if else_bb is
 	     empty forwarder block to then_bb.  Compared to the above simpler
@@ -1199,9 +1207,11 @@  tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
 	     edge from outer_cond_bb and the forwarder block.  */
 	  if (tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb, else_bb,
 				       then_bb, else_bb, bb))
-	    changed = true;
+	    changed = true, exit_pred = else_bb;
 	}
-      else if (forwarder_block_to (then_bb, else_bb))
+      else if (exit_bb
+	       ? exit_pred == then_bb
+	       : forwarder_block_to (then_bb, else_bb))
 	{
 	  /* Other possibilities for the || form, if then_bb is
 	     empty forwarder block to else_bb.  Compared to the above simpler
@@ -1211,7 +1221,7 @@  tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
 	     edge from outer_cond_bb and the forwarder block.  */
 	  if (tree_ssa_ifcombine_bb_1 (inner_cond_bb, outer_cond_bb, else_bb,
 				       then_bb, then_bb, bb))
-	    changed = true;
+	    changed = true, exit_pred = then_bb;
 	}
 
       if (changed)
@@ -1222,15 +1232,85 @@  tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
       if (changed && known_succ_p (inner_cond_bb))
 	break;
 
+      /* Starting at this point in the loop, we start preparing to attempt
+	 combinations in which OUTER_COND_BB will be an intervening block.
+	 Checking that it has a single predecessor is a very cheap test, unlike
+	 the PHI args tests below, so test it early and hopefully save the more
+	 expensive tests in case we won't be able to try other blocks.  */
+      if (!single_pred_p (outer_cond_bb))
+	break;
+
       /* Record the exit path taken by the outer condition.  */
       if (!exit_bb)
 	{
+	  /* 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
+	     the outer condition with a constant that leads away from the inner
+	     block.  Here's why we never do: when combining contiguous
+	     conditions, we replace the inner cond, and replace the outer cond
+	     with a constant that leads to inner, so this case is good.  When
+	     combining noncontiguous blocks, we normally modify outer, and
+	     replace inner with a constant or remainders of the original
+	     condition that couldn't be combined.  This test would normally not
+	     hit with noncontiguous blocks, because we'd have computed EXIT_BB
+	     before reaching the noncontiguous outer block.  However, if all
+	     intervening blocks are unconditional, including those just made
+	     unconditional, we may replace outer instead of inner with the
+	     combined condition.  If the combined noncontiguous conditions are
+	     mutually exclusive, we could end up with a constant outer
+	     condition, but then, the inner condition would also be a constant,
+	     and then we'd stop iterating because of the known_succ_p
+	     (inner_cond_bb) test above.  */
+	  if (changed && known_succ_p (outer_cond_bb))
+	    continue;
+
 	  if (recognize_if_then_else (outer_cond_bb, &then_bb, &bb, true))
 	    exit_bb = then_bb;
 	  else if (recognize_if_then_else (outer_cond_bb, &bb, &else_bb, true))
 	    exit_bb = else_bb;
 	  else
 	    break;
+
+	  /* Find out which path from INNER_COND_BB shares PHI args with the
+	     edge (OUTER_COND_BB->EXIT_BB).  That path may involve a forwarder
+	     block, whether THEN_BB or ELSE_BB, and we need to know which one
+	     satisfies the condition to avoid combinations that could use
+	     different forwarding arrangements, because they would be unsound.
+	     E.g., given (a ? 0 : b ? 1 : c ? 1 : 0), after trying to merge b
+	     and c, we test that both share the same exit block, with the same
+	     value 1.  Whether or not that involves a forwarder block, if we
+	     don't go through the same (possibly absent) forwarder block in
+	     subsequent attempted combinations, e.g. a with c, we could find
+	     that a and inverted c share the same exit block with a different
+	     value, namely 0, which would enable an unsound merge.  We need all
+	     of inner, intervening and outer blocks to reach the same exit with
+	     the same value for the transformation to be sound.  So here we
+	     determine how to get to EXIT_BB from outer and inner with the same
+	     PHI values, record that in EXIT_PRED, and then subsequent
+	     combination attempts that have OUTER_COND_BB as an intervening
+	     block will ensure the same path to exit is taken, skipping unsound
+	     transformations.  */
+	  if (changed)
+	    /* EXIT_PRED was set along with CHANGED, and the successful
+	       combination already checked for the same PHI args.  */;
+	  else if (same_phi_args_p (outer_cond_bb, inner_cond_bb, exit_bb))
+	    exit_pred = inner_cond_bb;
+	  else if (then_bb == exit_bb
+		   && forwarder_block_to (else_bb, then_bb)
+		   && same_phi_args_p (outer_cond_bb, else_bb, exit_bb))
+	    exit_pred = else_bb;
+	  else if (else_bb == exit_bb
+		   && forwarder_block_to (then_bb, else_bb)
+		   && same_phi_args_p (outer_cond_bb, then_bb, exit_bb))
+	    exit_pred = then_bb;
+	  else
+	    /* If none of the paths share the same PHI args, no combination is
+	       viable.  */
+	    break;
+	  /* Skip the PHI args test below, it's redundant with the tests we've
+	     just performed.  */
+	  continue;
 	}
 
       /* Before trying an earlier block, make sure INNER_COND_BB and the
@@ -1245,7 +1325,7 @@  tree_ssa_ifcombine_bb (basic_block inner_cond_bb)
 	 of outer, we'd get ([1 ? 0 :] b ? 1 : (a | c) ? 0 : -1), which would
 	 yield 1 rather than 0 when (a).  */
       if (!changed
-	  && !same_phi_args_p (outer_cond_bb, inner_cond_bb, exit_bb))
+	  && !same_phi_args_p (outer_cond_bb, exit_pred, exit_bb))
 	break;
     }