rtl-optimization: [PR102733] DSE removing address which only differ by address space.
Checks
Commit Message
The problem here is DSE was not taking into account the address space
which meant if you had two addresses say `fs:0` and `gs:0` (on x86_64),
DSE would think they were the same and remove the first store.
This fixes that issue by adding a check for the address space too.
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
PR rtl-optimization/102733
gcc/ChangeLog:
* dse.cc (store_info): Add addrspace field.
(record_store): Record the address space
and check to make sure they are the same.
gcc/testsuite/ChangeLog:
* gcc.target/i386/addr-space-6.c: New test.
---
gcc/dse.cc | 9 ++++++++-
gcc/testsuite/gcc.target/i386/addr-space-6.c | 21 ++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/i386/addr-space-6.c
Comments
On Fri, Jun 2, 2023 at 9:36 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The problem here is DSE was not taking into account the address space
> which meant if you had two addresses say `fs:0` and `gs:0` (on x86_64),
> DSE would think they were the same and remove the first store.
> This fixes that issue by adding a check for the address space too.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
OK.
> PR rtl-optimization/102733
>
> gcc/ChangeLog:
>
> * dse.cc (store_info): Add addrspace field.
> (record_store): Record the address space
> and check to make sure they are the same.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/i386/addr-space-6.c: New test.
> ---
> gcc/dse.cc | 9 ++++++++-
> gcc/testsuite/gcc.target/i386/addr-space-6.c | 21 ++++++++++++++++++++
> 2 files changed, 29 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/addr-space-6.c
>
> diff --git a/gcc/dse.cc b/gcc/dse.cc
> index 802b949cfb2..8b07be17674 100644
> --- a/gcc/dse.cc
> +++ b/gcc/dse.cc
> @@ -251,6 +251,9 @@ public:
> and known (rather than -1). */
> poly_int64 width;
>
> + /* The address space that the memory reference uses. */
> + unsigned char addrspace;
> +
> union
> {
> /* A bitmask as wide as the number of bytes in the word that
> @@ -1524,6 +1527,7 @@ record_store (rtx body, bb_info_t bb_info)
> ptr = active_local_stores;
> last = NULL;
> redundant_reason = NULL;
> + unsigned char addrspace = MEM_ADDR_SPACE (mem);
> mem = canon_rtx (mem);
>
> if (group_id < 0)
> @@ -1548,7 +1552,9 @@ record_store (rtx body, bb_info_t bb_info)
> while (!s_info->is_set)
> s_info = s_info->next;
>
> - if (s_info->group_id == group_id && s_info->cse_base == base)
> + if (s_info->group_id == group_id
> + && s_info->cse_base == base
> + && s_info->addrspace == addrspace)
> {
> HOST_WIDE_INT i;
> if (dump_file && (dump_flags & TDF_DETAILS))
> @@ -1688,6 +1694,7 @@ record_store (rtx body, bb_info_t bb_info)
> store_info->rhs = rhs;
> store_info->const_rhs = const_rhs;
> store_info->redundant_reason = redundant_reason;
> + store_info->addrspace = addrspace;
>
> /* If this is a clobber, we return 0. We will only be able to
> delete this insn if there is only one store USED store, but we
> diff --git a/gcc/testsuite/gcc.target/i386/addr-space-6.c b/gcc/testsuite/gcc.target/i386/addr-space-6.c
> new file mode 100644
> index 00000000000..82eca4d7e0c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/addr-space-6.c
> @@ -0,0 +1,21 @@
> +/* PR rtl-optimization/102733 */
> +/* { dg-do compile } */
> +/* { dg-options "-O1" } */
> +
> +/* DSE was removing a store to fs:0 (correctly)
> + and gs:0 (incorrectly) as DSE didn't take into
> + account the address space was different. */
> +
> +void test_null_store (void)
> +{
> + int __seg_fs *fs = (int __seg_fs *)0;
> + *fs = 1;
> +
> + int __seg_gs *gs = (int __seg_gs *)0;
> + *gs = 2;
> + *fs = 3;
> +}
> +
> +/* { dg-final { scan-assembler-times "movl\t" 2 } } */
> +/* { dg-final { scan-assembler "gs:" } } */
> +/* { dg-final { scan-assembler "fs:" } } */
> --
> 2.31.1
>
@@ -251,6 +251,9 @@ public:
and known (rather than -1). */
poly_int64 width;
+ /* The address space that the memory reference uses. */
+ unsigned char addrspace;
+
union
{
/* A bitmask as wide as the number of bytes in the word that
@@ -1524,6 +1527,7 @@ record_store (rtx body, bb_info_t bb_info)
ptr = active_local_stores;
last = NULL;
redundant_reason = NULL;
+ unsigned char addrspace = MEM_ADDR_SPACE (mem);
mem = canon_rtx (mem);
if (group_id < 0)
@@ -1548,7 +1552,9 @@ record_store (rtx body, bb_info_t bb_info)
while (!s_info->is_set)
s_info = s_info->next;
- if (s_info->group_id == group_id && s_info->cse_base == base)
+ if (s_info->group_id == group_id
+ && s_info->cse_base == base
+ && s_info->addrspace == addrspace)
{
HOST_WIDE_INT i;
if (dump_file && (dump_flags & TDF_DETAILS))
@@ -1688,6 +1694,7 @@ record_store (rtx body, bb_info_t bb_info)
store_info->rhs = rhs;
store_info->const_rhs = const_rhs;
store_info->redundant_reason = redundant_reason;
+ store_info->addrspace = addrspace;
/* If this is a clobber, we return 0. We will only be able to
delete this insn if there is only one store USED store, but we
new file mode 100644
@@ -0,0 +1,21 @@
+/* PR rtl-optimization/102733 */
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+/* DSE was removing a store to fs:0 (correctly)
+ and gs:0 (incorrectly) as DSE didn't take into
+ account the address space was different. */
+
+void test_null_store (void)
+{
+ int __seg_fs *fs = (int __seg_fs *)0;
+ *fs = 1;
+
+ int __seg_gs *gs = (int __seg_gs *)0;
+ *gs = 2;
+ *fs = 3;
+}
+
+/* { dg-final { scan-assembler-times "movl\t" 2 } } */
+/* { dg-final { scan-assembler "gs:" } } */
+/* { dg-final { scan-assembler "fs:" } } */