On 07/01/16 14:00, Kyrill Tkachov wrote:
On 07/01/16 13:47, Bernd Schmidt wrote:
On 01/07/2016 02:45 PM, Kyrill Tkachov wrote:
On 07/01/16 11:52, Bernd Schmidt wrote:
I looked and this is indeed the case. Still slightly scary. Should the
MEM be rtx_equal_p to TO_RENAME? It would be good to test or at least
assert this.
I tried asserting that and it caused trouble because a bit further up in
noce_process_if_block it does:
/* Only operate on register destinations, and even then avoid extending
the lifetime of hard registers on small register class machines. */
orig_x = x;
if (!REG_P (x)
|| (HARD_REGISTER_P (x)
&& targetm.small_register_classes_for_mode_p (GET_MODE (x))))
{
if (GET_MODE (x) == BLKmode)
return FALSE;
if (GET_CODE (x) == ZERO_EXTRACT
&& (!CONST_INT_P (XEXP (x, 1))
|| !CONST_INT_P (XEXP (x, 2))))
return FALSE;
x = gen_reg_rtx (GET_MODE (GET_CODE (x) == STRICT_LOW_PART
? XEXP (x, 0) : x));
}
It changes X to a register and after noce_try_cmove_arith it emits the
store.
This suggests that orig_x needs to be passed to bbs_ok_for_cmove_arith, even
disregarding the question of using it to assert that only expected MEMs are
being modified.
Yes, which should be the original destinations of insn_a and insn_b of if_info.
I'll work on that. Though if we execute the above path then x will be a fresh
new pseudo and so
will not cause any conflicts anyway.
Here is the latest version.
I passed down the original set destination to bbs_ok_for_cmove_arith and
asserted that it's the only memory store if a store occurs.
Bootstrapped and tested on arm, aarch64, x86_64.
How's this?
Thanks,
Kyrill
2016-01-08 Bernd Schmidt <bschm...@redhat.com>
Kyrylo Tkachov <kyrylo.tkac...@arm.com>
PR rtl-optimization/68841
* ifcvt.c (bbs_ok_for_cmove_arith): Add to_rename parameter.
Don't record conflicts on to_rename if it's present.
Allow memory destinations in sets.
(noce_try_cmove_arith): Call bbs_ok_for_cmove_arith even on simple
blocks.
2016-01-08 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
PR rtl-optimization/68841
* gcc.dg/pr68841.c: New test.
* gcc.c-torture/execute/pr68841.c: New test.
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 5812ce30151ed7425780890c66e7763f5758df7e..50fe69d13294ca1ddbac17cccecf436f7f9d231a 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1896,11 +1896,13 @@ insn_valid_noce_process_p (rtx_insn *insn, rtx cc)
}
-/* Return true iff the registers that the insns in BB_A set do not
- get used in BB_B. */
+/* Return true iff the registers that the insns in BB_A set do not get
+ used in BB_B. If TO_RENAME is non-NULL then it is a location that will be
+ renamed later by the caller and so conflicts on it should be ignored
+ in this function. */
static bool
-bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b)
+bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b, rtx to_rename)
{
rtx_insn *a_insn;
bitmap bba_sets = BITMAP_ALLOC (®_obstack);
@@ -1920,10 +1922,10 @@ bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b)
BITMAP_FREE (bba_sets);
return false;
}
-
/* Record all registers that BB_A sets. */
FOR_EACH_INSN_DEF (def, a_insn)
- bitmap_set_bit (bba_sets, DF_REF_REGNO (def));
+ if (!(to_rename && DF_REF_REG (def) == to_rename))
+ bitmap_set_bit (bba_sets, DF_REF_REGNO (def));
}
rtx_insn *b_insn;
@@ -1942,8 +1944,15 @@ bbs_ok_for_cmove_arith (basic_block bb_a, basic_block bb_b)
}
/* Make sure this is a REG and not some instance
- of ZERO_EXTRACT or SUBREG or other dangerous stuff. */
- if (!REG_P (SET_DEST (sset_b)))
+ of ZERO_EXTRACT or SUBREG or other dangerous stuff.
+ If we have a memory destination then we have a pair of simple
+ basic blocks performing an operation of the form [addr] = c ? a : b.
+ bb_valid_for_noce_process_p will have ensured that these are
+ the only stores present. In that case [addr] should be the location
+ to be renamed. Assert that the callers set this up properly. */
+ if (MEM_P (SET_DEST (sset_b)))
+ gcc_assert (rtx_equal_p (SET_DEST (sset_b), to_rename));
+ else if (!REG_P (SET_DEST (sset_b)))
{
BITMAP_FREE (bba_sets);
return false;
@@ -2112,9 +2121,16 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
}
}
- if (then_bb && else_bb && !a_simple && !b_simple
- && (!bbs_ok_for_cmove_arith (then_bb, else_bb)
- || !bbs_ok_for_cmove_arith (else_bb, then_bb)))
+ rtx orig_x = x;
+ if (then_bb && else_bb)
+ {
+ orig_x = SET_DEST (single_set (insn_a));
+ gcc_assert (rtx_equal_p (orig_x, SET_DEST (single_set (insn_b))));
+ }
+
+ if (then_bb && else_bb
+ && (!bbs_ok_for_cmove_arith (then_bb, else_bb, orig_x)
+ || !bbs_ok_for_cmove_arith (else_bb, then_bb, orig_x)))
return FALSE;
start_sequence ();
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68841.c b/gcc/testsuite/gcc.c-torture/execute/pr68841.c
new file mode 100644
index 0000000000000000000000000000000000000000..15a27e7dc382d97398ca05427f431f5ecd3b89da
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68841.c
@@ -0,0 +1,31 @@
+static inline int
+foo (int *x, int y)
+{
+ int z = *x;
+ while (y > z)
+ z *= 2;
+ return z;
+}
+
+int
+main ()
+{
+ int i;
+ for (i = 1; i < 17; i++)
+ {
+ int j;
+ int k;
+ j = foo (&i, 7);
+ if (i >= 7)
+ k = i;
+ else if (i >= 4)
+ k = 8 + (i - 4) * 2;
+ else if (i == 3)
+ k = 12;
+ else
+ k = 8;
+ if (j != k)
+ __builtin_abort ();
+ }
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr68841.c b/gcc/testsuite/gcc.dg/pr68841.c
new file mode 100644
index 0000000000000000000000000000000000000000..470048cc24f0d7150ed1e3141181bc1e8472ae12
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr68841.c
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-Og -fif-conversion -flive-range-shrinkage -fpeel-loops -frerun-cse-after-loop" } */
+
+static inline int
+foo (int *x, int y)
+{
+ int z = *x;
+ while (y > z)
+ z *= 2;
+ return z;
+}
+
+int
+main ()
+{
+ int i;
+ for (i = 1; i < 17; i++)
+ {
+ int j;
+ int k;
+ j = foo (&i, 7);
+ if (i >= 7)
+ k = i;
+ else if (i >= 4)
+ k = 8 + (i - 4) * 2;
+ else if (i == 3)
+ k = 12;
+ else
+ k = 8;
+ if (j != k)
+ __builtin_abort ();
+ }
+ return 0;
+}
