Hi all,
In this fix I want to simplify the control flow of the code that chooses the
order in which to emit
the then and else basic blocks (and their associated emit_a and emit_b
instructions).
Currently we check the then block and only if there is a modification there we
check the else block
and make a decision there. IMO it's much simpler if we check both blocks and
write the logic that
chooses the order as a simple IF-ELSEIF-ELSE block that only emits the blocks
and doesn't try to do
any other checks. The bug in the logic that was preventing the clobber check
from being performed
in this PR was in the code:
if (emit_a || modified_in_a)
{
modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
if (tmp_b && else_bb)
{
FOR_BB_INSNS (else_bb, tmp_insn)
where the second if condition should have been:
if (tmp_a && else_bb)
Just changing the tmp_b to tmp_a in that condition would have fixed the
wrong-code part of this PR
as we would have ended up rejecting if-conversion. However, there is a valid
if-conversion opportunity
here, we just have to emit emit_a followed by else_bb, which the current
control flow made awkward, which
is why I'm suggesting this small rewrite.
Bootstrapped and tested on x86_64, aarch64, arm.
Ok for trunk?
Thanks,
Kyrill
2015-12-03 Kyrylo Tkachov
PR rtl-optimization/68624
* ifcvt.c (noce_try_cmove_arith): Check clobbers of temp regs in both
blocks if they exist and simplify the logic choosing the order to emit
them in.
2015-12-03 Kyrylo Tkachov
PR rtl-optimization/68624
* gcc.c-torture/execute/pr68624.c: New test.
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 86b6ef7246ceddd223e93922737496af3d93f148..ef23c4cda66e6a659eee9b30089a6cc056cea30f 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2202,10 +2202,6 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
-/* If insn to set up A clobbers any registers B depends on, try to
- swap insn that sets up A with the one that sets up B. If even
- that doesn't help, punt. */
-
modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
if (tmp_b && then_bb)
{
@@ -2220,31 +2216,33 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
- if (emit_a || modified_in_a)
+
+ modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
+ if (tmp_a && else_bb)
{
- modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
- if (tmp_b && else_bb)
+ FOR_BB_INSNS (else_bb, tmp_insn)
+ /* Don't check inside insn_b. We will have changed it to emit_b
+ with a destination that doesn't conflict. */
+ if (!(insn_b && tmp_insn == insn_b)
+ && modified_in_p (orig_a, tmp_insn))
{
- FOR_BB_INSNS (else_bb, tmp_insn)
- /* Don't check inside insn_b. We will have changed it to emit_b
- with a destination that doesn't conflict. */
- if (!(insn_b && tmp_insn == insn_b)
- && modified_in_p (orig_a, tmp_insn))
- {
- modified_in_b = true;
- break;
- }
+ modified_in_b = true;
+ break;
}
- if (modified_in_b)
- goto end_seq_and_fail;
+}
+ /* If insn to set up A clobbers any registers B depends on, try to
+ swap insn that sets up A with the one that sets up B. If even
+ that doesn't help, punt. */
+ if (modified_in_a && !modified_in_b)
+{
if (!noce_emit_bb (emit_b, else_bb, b_simple))
goto end_seq_and_fail;
if (!noce_emit_bb (emit_a, then_bb, a_simple))
goto end_seq_and_fail;
}
- else
+ else if (!modified_in_a)
{
if (!noce_emit_bb (emit_a, then_bb, a_simple))
goto end_seq_and_fail;
@@ -2252,6 +2250,8 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
if (!noce_emit_bb (emit_b, else_bb, b_simple))
goto end_seq_and_fail;
}
+ else
+goto end_seq_and_fail;
target = noce_emit_cmove (if_info, x, code, XEXP (if_info->cond, 0),
XEXP (if_info->cond, 1), a, b);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68624.c b/gcc/testsuite/gcc.c-torture/execute/pr68624.c
new file mode 100644
index ..abb716b1550038cb3d0e96e8917b7ed0ba8bfa83
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68624.c
@@ -0,0 +1,30 @@
+int b, c, d, e = 1, f, g, h, j;
+
+static int
+fn1 ()
+{
+ int a = c;
+ if (h)
+return 9;
+ g = (c || b) % e;
+ if ((g || f) && b)
+return 9;
+ e = d;
+ for (c = 0; c > -4; c--)
+;
+ if (d)
+c--;
+ j = c;
+ return d;
+}
+
+int
+main ()
+{
+ fn1 ();
+
+ if (c != -4)
+__builtin_abort ();
+
+ return 0;
+}
