Message ID | 20161201153008.GA28115@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote: > gcc/ChangeLog > > * combine.c (combine_simplify_rtx): Suppress replacement of > "(and (reg) (const_int bit))" with "if_then_else". Applied. Thanks! -Andreas-
On Dez 01 2016, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote: > gcc/ChangeLog > > * combine.c (combine_simplify_rtx): Suppress replacement of > "(and (reg) (const_int bit))" with "if_then_else". That causes a regression on ia64. FAIL: gcc.target/ia64/builtin-popcount-2.c scan-assembler-not popcnt Andreas.
[ I did not see this patch before, sorry. ] This causes the second half of PR78638. On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote: > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, > && OBJECT_P (SUBREG_REG (XEXP (x, 0))))))) > { > rtx cond, true_rtx, false_rtx; > + unsigned HOST_WIDE_INT nz; > + > + /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with > + either operand being just a constant single bit value, do nothing since > + IF_THEN_ELSE is likely to increase the expression's complexity. */ > + if (HWI_COMPUTABLE_MODE_P (mode) > + && pow2p_hwi (nz = nonzero_bits (x, mode)) > + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) > + && GET_CODE (XEXP (x, 0)) == AND > + && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) > + return x; The code does not match the comment: the "!" should not be there. How did it fix anything on s390 *with* that "!"? That does not make much sense. Segher
On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote: > [ I did not see this patch before, sorry. ] > > This causes the second half of PR78638. > > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote: > > --- a/gcc/combine.c > > +++ b/gcc/combine.c > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, > > && OBJECT_P (SUBREG_REG (XEXP (x, 0))))))) > > { > > rtx cond, true_rtx, false_rtx; > > + unsigned HOST_WIDE_INT nz; > > + > > + /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with > > + either operand being just a constant single bit value, do nothing since > > + IF_THEN_ELSE is likely to increase the expression's complexity. */ > > + if (HWI_COMPUTABLE_MODE_P (mode) > > + && pow2p_hwi (nz = nonzero_bits (x, mode)) > > + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) > > + && GET_CODE (XEXP (x, 0)) == AND > > + && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > > + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) > > + return x; > > The code does not match the comment: the "!" should not be there. How > did it fix anything on s390 *with* that "!"? That does not make much > sense. Sorry for breaking this. With the constant changes in the patterns this is supposed to fix it seems I've lost track of the status quo. I'll check what went wrong with the patch; in the mean time Andreas will revert this, or if it's urgent, feel free to do that yourself. Ciao Dominik ^_^ ^_^
On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote: > On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote: > > [ I did not see this patch before, sorry. ] > > > > This causes the second half of PR78638. > > > > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote: > > > --- a/gcc/combine.c > > > +++ b/gcc/combine.c > > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, > > > && OBJECT_P (SUBREG_REG (XEXP (x, 0))))))) > > > { > > > rtx cond, true_rtx, false_rtx; > > > + unsigned HOST_WIDE_INT nz; > > > + > > > + /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with > > > + either operand being just a constant single bit value, do nothing since > > > + IF_THEN_ELSE is likely to increase the expression's complexity. */ > > > + if (HWI_COMPUTABLE_MODE_P (mode) > > > + && pow2p_hwi (nz = nonzero_bits (x, mode)) > > > + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) > > > + && GET_CODE (XEXP (x, 0)) == AND > > > + && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > > > + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) > > > + return x; > > > > The code does not match the comment: the "!" should not be there. How > > did it fix anything on s390 *with* that "!"? That does not make much > > sense. > > Sorry for breaking this. With the constant changes in the > patterns this is supposed to fix it seems I've lost track of the > status quo. I'll check what went wrong with the patch; in the > mean time Andreas will revert this, or if it's urgent, feel free > to do that yourself. I have tested that removing that ! cures all regressions. I do not know if it still fixes what this patch intended to fix, of course. Segher
On Mon, Dec 05, 2016 at 04:00:25AM -0600, Segher Boessenkool wrote: > On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote: > > On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote: > > > [ I did not see this patch before, sorry. ] > > > > > > This causes the second half of PR78638. > > > > > > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote: > > > > --- a/gcc/combine.c > > > > +++ b/gcc/combine.c > > > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, > > > > && OBJECT_P (SUBREG_REG (XEXP (x, 0))))))) > > > > { > > > > rtx cond, true_rtx, false_rtx; > > > > + unsigned HOST_WIDE_INT nz; > > > > + > > > > + /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with > > > > + either operand being just a constant single bit value, do nothing since > > > > + IF_THEN_ELSE is likely to increase the expression's complexity. */ > > > > + if (HWI_COMPUTABLE_MODE_P (mode) > > > > + && pow2p_hwi (nz = nonzero_bits (x, mode)) > > > > + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) > > > > + && GET_CODE (XEXP (x, 0)) == AND > > > > + && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > > > > + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) > > > > + return x; > > > > > > The code does not match the comment: the "!" should not be there. How > > > did it fix anything on s390 *with* that "!"? That does not make much > > > sense. > > > > Sorry for breaking this. With the constant changes in the > > patterns this is supposed to fix it seems I've lost track of the > > status quo. I'll check what went wrong with the patch; in the > > mean time Andreas will revert this, or if it's urgent, feel free > > to do that yourself. > > I have tested that removing that ! cures all regressions. I do not > know if it still fixes what this patch intended to fix, of course. S390[x] has these complicated patterns for the risbg instruction, and there's some ongoing work on patterns for related instructions (rosbg, rxsbg) which needed the patch discussed here - at least at some point in time. But the risbg patterns are breaking all over the place because they are so sensitive to changes in combine.c (and possibly other passes), and any change fixing the old patterns may affect the new ones. In other words: At the moment I have no clue whether the discussed patch is still good for anythin on s390[x]. If there was a consensus that the patch discussed here, with the "!" fix is useful in any case, that would simplify my current work, but 1) I've done no testing with it (only with the broken version of the patch), 2) it may be just a chunk of dead code. Ciao Dominik ^_^ ^_^
On Mon, 2016-12-05 at 04:00 -0600, Segher Boessenkool wrote: > On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote: > > > > On Sat, Dec 03, 2016 at 07:19:13PM -0600, Segher Boessenkool wrote: > > > > > > [ I did not see this patch before, sorry. ] > > > > > > This causes the second half of PR78638. > > > > > > On Thu, Dec 01, 2016 at 04:30:08PM +0100, Dominik Vogt wrote: > > > > > > > > --- a/gcc/combine.c > > > > +++ b/gcc/combine.c > > > > @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, > > > > machine_mode op0_mode, int in_dest, > > > > && OBJECT_P (SUBREG_REG (XEXP (x, > > > > 0))))))) > > > > { > > > > rtx cond, true_rtx, false_rtx; > > > > + unsigned HOST_WIDE_INT nz; > > > > + > > > > + /* If the operation is an AND wrapped in a SIGN_EXTEND > > > > or ZERO_EXTEND with > > > > + either operand being just a constant single bit > > > > value, do nothing since > > > > + IF_THEN_ELSE is likely to increase the expression's > > > > complexity. */ > > > > + if (HWI_COMPUTABLE_MODE_P (mode) > > > > + && pow2p_hwi (nz = nonzero_bits (x, mode)) > > > > + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) > > > > + && GET_CODE (XEXP (x, 0)) == AND > > > > + && CONST_INT_P (XEXP (XEXP (x, 0), 0)) > > > > + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) > > > > + return x; > > > The code does not match the comment: the "!" should not be > > > there. How > > > did it fix anything on s390 *with* that "!"? That does not make > > > much > > > sense. > > Sorry for breaking this. With the constant changes in the > > patterns this is supposed to fix it seems I've lost track of the > > status quo. I'll check what went wrong with the patch; in the > > mean time Andreas will revert this, or if it's urgent, feel free > > to do that yourself. > I have tested that removing that ! cures all regressions. I do not > know if it still fixes what this patch intended to fix, of course. I haven't been following this, but it seems some of these changes also triggered bleh on SH: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78633 Cheers, Oleg
On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote: > Sorry for breaking this. With the constant changes in the > patterns this is supposed to fix it seems I've lost track of the > status quo. I'll check what went wrong with the patch; in the > mean time Andreas will revert this, or if it's urgent, feel free > to do that yourself. I've reverted it now, r243256. Thanks, Segher
On Mon, Dec 05, 2016 at 07:56:46AM -0600, Segher Boessenkool wrote: > On Mon, Dec 05, 2016 at 10:22:13AM +0100, Dominik Vogt wrote: > > Sorry for breaking this. With the constant changes in the > > patterns this is supposed to fix it seems I've lost track of the > > status quo. I'll check what went wrong with the patch; in the > > mean time Andreas will revert this, or if it's urgent, feel free > > to do that yourself. > > I've reverted it now, r243256. Thanks. I need to think about this patch and the patch that is based on it for a while, so there's no need to get the fixed patch into svn for now. Ciao Dominik ^_^ ^_^
diff --git a/gcc/combine.c b/gcc/combine.c index a8dae89..52bde9e 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -5600,6 +5600,18 @@ combine_simplify_rtx (rtx x, machine_mode op0_mode, int in_dest, && OBJECT_P (SUBREG_REG (XEXP (x, 0))))))) { rtx cond, true_rtx, false_rtx; + unsigned HOST_WIDE_INT nz; + + /* If the operation is an AND wrapped in a SIGN_EXTEND or ZERO_EXTEND with + either operand being just a constant single bit value, do nothing since + IF_THEN_ELSE is likely to increase the expression's complexity. */ + if (HWI_COMPUTABLE_MODE_P (mode) + && pow2p_hwi (nz = nonzero_bits (x, mode)) + && ! ((code == SIGN_EXTEND || code == ZERO_EXTEND) + && GET_CODE (XEXP (x, 0)) == AND + && CONST_INT_P (XEXP (XEXP (x, 0), 0)) + && UINTVAL (XEXP (XEXP (x, 0), 0)) == nz)) + return x; cond = if_then_else_cond (x, &true_rtx, &false_rtx); if (cond != 0