Message ID | 568E2E6C.4050806@foss.arm.com |
---|---|
State | New |
Headers | show |
On 01/07/2016 10:22 AM, Kyrill Tkachov wrote: > +/* 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 REG that will be > + renamed later by the caller and so conflicts on it should be ignored > + in this function. */ Typical spelling is "non-NULL". > @@ -1942,8 +1944,12 @@ 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. */ 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. Bernd
On 07/01/16 11:52, Bernd Schmidt wrote: > On 01/07/2016 10:22 AM, Kyrill Tkachov wrote: >> +/* 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 REG that will be >> + renamed later by the caller and so conflicts on it should be ignored >> + in this function. */ > > Typical spelling is "non-NULL". > >> @@ -1942,8 +1944,12 @@ 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. */ > > 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. So, while the basic blocks that we inspect in bbs_ok_for_cmove_arith contain memory destinations, the move to x will be a register move. move will be performed t Kyrill > > Bernd
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. Bernd
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. Kyrill > > Bernd
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 5812ce30151ed7425780890c66e7763f5758df7e..96957aac5481acbc7ac5f186a653fe63ad3e5f51 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 REG 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,12 @@ 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. */ + if (!REG_P (SET_DEST (sset_b)) && !MEM_P (SET_DEST (sset_b))) { BITMAP_FREE (bba_sets); return false; @@ -2112,9 +2118,9 @@ 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))) + if (then_bb && else_bb + && (!bbs_ok_for_cmove_arith (then_bb, else_bb, x) + || !bbs_ok_for_cmove_arith (else_bb, then_bb, 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; +}