* [PATCH] combine: Count auto_inc properly (PR89794)
@ 2019-04-14 9:56 Segher Boessenkool
2019-04-15 11:48 ` Segher Boessenkool
0 siblings, 1 reply; 2+ messages in thread
From: Segher Boessenkool @ 2019-04-14 9:56 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool
The code that checks if an auto-increment from i0 or i1 is not lost is
a bit shaky. The code to check the same for i2 is non-existent, and
cannot be implemented in a similar way at all. So, this patch counts
all auto-increments, and makes sure we end up with the same number as
we started with. This works because we still have a check that we
will not duplicate any.
We should do this some better way, but not while we are in stage 4.
Tested on powerpc64-linux {-m32,-m64}; also tested manually on the Arm
testcase.
Comments?
Segher
2019-04-14 Segher Boessenkool <segher@kernel.crashing.org>
PR rtl-optimization/89794
* combine.c (count_auto_inc): New function.
(try_combine): Count how many auto_inc expressions there were in the
original instructions. Ensure we have the same number in the new
instructions. Remove the code that tried to ensure auto_inc side
effects on i1 and i0 are not lost.
---
gcc/combine.c | 60 +++++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/gcc/combine.c b/gcc/combine.c
index f681345..091adea 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2667,6 +2667,16 @@ combine_remove_reg_equal_equiv_notes_for_regno (unsigned int regno)
}
}
+/* Callback function to count autoincs. */
+
+int
+count_auto_inc (rtx, rtx, rtx, rtx, rtx, void *arg)
+{
+ (*((int *) arg))++;
+
+ return 0;
+}
+
/* Try to combine the insns I0, I1 and I2 into I3.
Here I0, I1 and I2 appear earlier than I3.
I0 and I1 can be zero; then we combine just I2 into I3, or I1 and I2 into
@@ -2732,6 +2742,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
int split_i2i3 = 0;
int changed_i3_dest = 0;
bool i2_was_move = false, i3_was_move = false;
+ int n_auto_inc = 0;
int maxreg;
rtx_insn *temp_insn;
@@ -3236,6 +3247,16 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
return 0;
}
+ /* Count how many auto_inc expressions there were in the original insns;
+ we need to have the same number in the resulting patterns. */
+
+ if (i0)
+ for_each_inc_dec (PATTERN (i0), count_auto_inc, &n_auto_inc);
+ if (i1)
+ for_each_inc_dec (PATTERN (i1), count_auto_inc, &n_auto_inc);
+ for_each_inc_dec (PATTERN (i2), count_auto_inc, &n_auto_inc);
+ for_each_inc_dec (PATTERN (i3), count_auto_inc, &n_auto_inc);
+
/* If the set in I2 needs to be kept around, we must make a copy of
PATTERN (I2), so that when we substitute I1SRC for I1DEST in
PATTERN (I2), we are only substituting for the original I1DEST, not into
@@ -3439,18 +3460,11 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
if (i1 && GET_CODE (newpat) != CLOBBER)
{
- /* Check that an autoincrement side-effect on I1 has not been lost.
- This happens if I1DEST is mentioned in I2 and dies there, and
- has disappeared from the new pattern. */
- if ((FIND_REG_INC_NOTE (i1, NULL_RTX) != 0
- && i1_feeds_i2_n
- && dead_or_set_p (i2, i1dest)
- && !reg_overlap_mentioned_p (i1dest, newpat))
- /* Before we can do this substitution, we must redo the test done
- above (see detailed comments there) that ensures I1DEST isn't
- mentioned in any SETs in NEWPAT that are field assignments. */
- || !combinable_i3pat (NULL, &newpat, i1dest, NULL_RTX, NULL_RTX,
- 0, 0, 0))
+ /* Before we can do this substitution, we must redo the test done
+ above (see detailed comments there) that ensures I1DEST isn't
+ mentioned in any SETs in NEWPAT that are field assignments. */
+ if (!combinable_i3pat (NULL, &newpat, i1dest, NULL_RTX, NULL_RTX,
+ 0, 0, 0))
{
undo_all ();
return 0;
@@ -3480,12 +3494,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
if (i0 && GET_CODE (newpat) != CLOBBER)
{
- if ((FIND_REG_INC_NOTE (i0, NULL_RTX) != 0
- && ((i0_feeds_i2_n && dead_or_set_p (i2, i0dest))
- || (i0_feeds_i1_n && dead_or_set_p (i1, i0dest)))
- && !reg_overlap_mentioned_p (i0dest, newpat))
- || !combinable_i3pat (NULL, &newpat, i0dest, NULL_RTX, NULL_RTX,
- 0, 0, 0))
+ if (!combinable_i3pat (NULL, &newpat, i0dest, NULL_RTX, NULL_RTX,
+ 0, 0, 0))
{
undo_all ();
return 0;
@@ -3506,6 +3516,20 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
substed_i0 = 1;
}
+ if (n_auto_inc)
+ {
+ int new_n_auto_inc = 0;
+ for_each_inc_dec (newpat, count_auto_inc, &new_n_auto_inc);
+
+ if (n_auto_inc != new_n_auto_inc)
+ {
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file, "Number of auto_inc expressions changed\n");
+ undo_all ();
+ return 0;
+ }
+ }
+
/* Fail if an autoincrement side-effect has been duplicated. Be careful
to count all the ways that I2SRC and I1SRC can be used. */
if ((FIND_REG_INC_NOTE (i2, NULL_RTX) != 0
--
1.8.3.1
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] combine: Count auto_inc properly (PR89794)
2019-04-14 9:56 [PATCH] combine: Count auto_inc properly (PR89794) Segher Boessenkool
@ 2019-04-15 11:48 ` Segher Boessenkool
0 siblings, 0 replies; 2+ messages in thread
From: Segher Boessenkool @ 2019-04-15 11:48 UTC (permalink / raw)
To: gcc-patches
On Sun, Apr 14, 2019 at 09:51:39AM +0000, Segher Boessenkool wrote:
> The code that checks if an auto-increment from i0 or i1 is not lost is
> a bit shaky. The code to check the same for i2 is non-existent, and
> cannot be implemented in a similar way at all. So, this patch counts
> all auto-increments, and makes sure we end up with the same number as
> we started with. This works because we still have a check that we
> will not duplicate any.
>
> We should do this some better way, but not while we are in stage 4.
>
> Tested on powerpc64-linux {-m32,-m64}; also tested manually on the Arm
> testcase.
I added a missing "static", and added the testcase, as attached.
Committing it now.
Subject: [PATCH] combine: Count auto_inc properly (PR89794)
The code that checks if an auto-increment from i0 or i1 is not lost is
a bit shaky. The code to check the same for i2 is non-existent, and
cannot be implemented in a similar way at all. So, this patch counts
all auto-increments, and makes sure we end up with the same number as
we started with. This works because we still have a check that we
will not duplicate any.
2019-04-15 Segher Boessenkool <segher@kernel.crashing.org>
PR rtl-optimization/89794
* combine.c (count_auto_inc): New function.
(try_combine): Count how many auto_inc expressions there were in the
original instructions. Ensure we have the same number in the new
instructions. Remove the code that tried to ensure auto_inc side
effects on i1 and i0 are not lost.
gcc/testsuite/
PR rtl-optimization/89794
* gcc.dg/torture/pr89794.c: New testcase.
---
gcc/combine.c | 60 ++++++++++++++++++++++++----------
gcc/testsuite/gcc.dg/torture/pr89794.c | 24 ++++++++++++++
2 files changed, 66 insertions(+), 18 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/torture/pr89794.c
diff --git a/gcc/combine.c b/gcc/combine.c
index f681345..07bd0cf 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2667,6 +2667,16 @@ combine_remove_reg_equal_equiv_notes_for_regno (unsigned int regno)
}
}
+/* Callback function to count autoincs. */
+
+static int
+count_auto_inc (rtx, rtx, rtx, rtx, rtx, void *arg)
+{
+ (*((int *) arg))++;
+
+ return 0;
+}
+
/* Try to combine the insns I0, I1 and I2 into I3.
Here I0, I1 and I2 appear earlier than I3.
I0 and I1 can be zero; then we combine just I2 into I3, or I1 and I2 into
@@ -2732,6 +2742,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
int split_i2i3 = 0;
int changed_i3_dest = 0;
bool i2_was_move = false, i3_was_move = false;
+ int n_auto_inc = 0;
int maxreg;
rtx_insn *temp_insn;
@@ -3236,6 +3247,16 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
return 0;
}
+ /* Count how many auto_inc expressions there were in the original insns;
+ we need to have the same number in the resulting patterns. */
+
+ if (i0)
+ for_each_inc_dec (PATTERN (i0), count_auto_inc, &n_auto_inc);
+ if (i1)
+ for_each_inc_dec (PATTERN (i1), count_auto_inc, &n_auto_inc);
+ for_each_inc_dec (PATTERN (i2), count_auto_inc, &n_auto_inc);
+ for_each_inc_dec (PATTERN (i3), count_auto_inc, &n_auto_inc);
+
/* If the set in I2 needs to be kept around, we must make a copy of
PATTERN (I2), so that when we substitute I1SRC for I1DEST in
PATTERN (I2), we are only substituting for the original I1DEST, not into
@@ -3439,18 +3460,11 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
if (i1 && GET_CODE (newpat) != CLOBBER)
{
- /* Check that an autoincrement side-effect on I1 has not been lost.
- This happens if I1DEST is mentioned in I2 and dies there, and
- has disappeared from the new pattern. */
- if ((FIND_REG_INC_NOTE (i1, NULL_RTX) != 0
- && i1_feeds_i2_n
- && dead_or_set_p (i2, i1dest)
- && !reg_overlap_mentioned_p (i1dest, newpat))
- /* Before we can do this substitution, we must redo the test done
- above (see detailed comments there) that ensures I1DEST isn't
- mentioned in any SETs in NEWPAT that are field assignments. */
- || !combinable_i3pat (NULL, &newpat, i1dest, NULL_RTX, NULL_RTX,
- 0, 0, 0))
+ /* Before we can do this substitution, we must redo the test done
+ above (see detailed comments there) that ensures I1DEST isn't
+ mentioned in any SETs in NEWPAT that are field assignments. */
+ if (!combinable_i3pat (NULL, &newpat, i1dest, NULL_RTX, NULL_RTX,
+ 0, 0, 0))
{
undo_all ();
return 0;
@@ -3480,12 +3494,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
if (i0 && GET_CODE (newpat) != CLOBBER)
{
- if ((FIND_REG_INC_NOTE (i0, NULL_RTX) != 0
- && ((i0_feeds_i2_n && dead_or_set_p (i2, i0dest))
- || (i0_feeds_i1_n && dead_or_set_p (i1, i0dest)))
- && !reg_overlap_mentioned_p (i0dest, newpat))
- || !combinable_i3pat (NULL, &newpat, i0dest, NULL_RTX, NULL_RTX,
- 0, 0, 0))
+ if (!combinable_i3pat (NULL, &newpat, i0dest, NULL_RTX, NULL_RTX,
+ 0, 0, 0))
{
undo_all ();
return 0;
@@ -3506,6 +3516,20 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
substed_i0 = 1;
}
+ if (n_auto_inc)
+ {
+ int new_n_auto_inc = 0;
+ for_each_inc_dec (newpat, count_auto_inc, &new_n_auto_inc);
+
+ if (n_auto_inc != new_n_auto_inc)
+ {
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file, "Number of auto_inc expressions changed\n");
+ undo_all ();
+ return 0;
+ }
+ }
+
/* Fail if an autoincrement side-effect has been duplicated. Be careful
to count all the ways that I2SRC and I1SRC can be used. */
if ((FIND_REG_INC_NOTE (i2, NULL_RTX) != 0
diff --git a/gcc/testsuite/gcc.dg/torture/pr89794.c b/gcc/testsuite/gcc.dg/torture/pr89794.c
new file mode 100644
index 0000000..91bb0c4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr89794.c
@@ -0,0 +1,24 @@
+/* { dg-do run } */
+
+typedef unsigned short u16;
+typedef unsigned int u32;
+typedef unsigned long long u64;
+
+u32 a, b, c, d;
+
+u32 foo (u32 f, u32 g, u32 g2, u32 g3, u16 h, u16 i)
+{
+ (void)g, (void)g2, (void)g3, (void)h;
+ d = __builtin_bswap64 (i);
+ __builtin_sub_overflow (0, d, &b);
+ __builtin_memset (&i, c, 2);
+ a = 0;
+ return b + f + i + c;
+}
+
+int main (void)
+{
+ u32 x = foo (0, 0, 0, 0, 0, 0);
+ asm ("" :: "r" (x));
+ return 0;
+}
--
1.8.3.1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-04-15 11:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-14 9:56 [PATCH] combine: Count auto_inc properly (PR89794) Segher Boessenkool
2019-04-15 11:48 ` Segher Boessenkool
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).