* [PATCH] [FIX] Remove object adjustment to preserve object attributes
@ 2020-08-15 22:53 Petro Karashchenko
2020-08-20 8:01 ` Richard Sandiford
0 siblings, 1 reply; 4+ messages in thread
From: Petro Karashchenko @ 2020-08-15 22:53 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]
for bitfield MEMREFs
[FIX] Propagate uncached type attributes to unaligned/packed types
[ARC] Update tests
gcc/
xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
* emit-rtl.c (adjust_address_1): Do not drop the object
if the new memory reference is outside the underlying
object to preserve object attributes that are needed
by some backend implementations. Remove adjust_object
parameter as it is not used anymore.
(adjust_automodify_address_1): Adjust according to
new adjust_address_1 prototype.
(replace_equiv_address_nv): Likewise.
* gcc/emit-rtl.h (adjust_address): Adjust according to
new adjust_address_1 prototype.
(adjust_address_nv): Likewise.
(adjust_bitfield_address): Likewise.
(adjust_bitfield_address_size): Likewise.
(adjust_bitfield_address_nv): Likewise.
testsuite/
xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
* gcc.target/arc/uncached-9.c: New file.
Problem description:
__attribute__((uncached)) is dropped for other than the first member of
"packed" or "unaligned" types.
Tests:
Tested with ARC600 bases ASIC. The correct code is generated.
[-- Attachment #2: 0001-FIX-Remove-object-adjustment-to-preserve-object-attr.patch --]
[-- Type: application/octet-stream, Size: 7228 bytes --]
From 1f8a824f8ed8c1452f2bdfc513716c63d5d12545 Mon Sep 17 00:00:00 2001
From: Petro Karashchenko <petro.karashchenko@gmail.com>
Date: Sun, 16 Aug 2020 01:10:11 +0300
Subject: [PATCH] [FIX] Remove object adjustment to preserve object attributes
for bitfield MEMREFs
[FIX] Propagate uncached type attributes to unaligned/packed types
[ARC] Update tests
gcc/
xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
* emit-rtl.c (adjust_address_1): Do not drop the object
if the new memory reference is outside the underlying
object to preserve object attributes that are needed
by some backend implementations. Remove adjust_object
parameter as it is not used anymore.
(adjust_automodify_address_1): Adjust according to
new adjust_address_1 prototype.
(replace_equiv_address_nv): Likewise.
* gcc/emit-rtl.h (adjust_address): Adjust according to
new adjust_address_1 prototype.
(adjust_address_nv): Likewise.
(adjust_bitfield_address): Likewise.
(adjust_bitfield_address_size): Likewise.
(adjust_bitfield_address_nv): Likewise.
testsuite/
xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
* gcc.target/arc/uncached-9.c: New file.
Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
---
gcc/emit-rtl.c | 28 +++--------------------
gcc/emit-rtl.h | 12 +++++-----
gcc/testsuite/gcc.target/arc/uncached-9.c | 19 +++++++++++++++
3 files changed, 28 insertions(+), 31 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/arc/uncached-9.c
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index f9b0e9714d9..c0e2ead907b 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -2337,8 +2337,7 @@ change_address (rtx memref, machine_mode mode, rtx addr)
rtx
adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
- int validate, int adjust_address, int adjust_object,
- poly_int64 size)
+ int validate, int adjust_address, poly_int64 size)
{
rtx addr = XEXP (memref, 0);
rtx new_rtx;
@@ -2413,25 +2412,11 @@ adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
if (new_rtx == memref && maybe_ne (offset, 0))
new_rtx = copy_rtx (new_rtx);
- /* Conservatively drop the object if we don't know where we start from. */
- if (adjust_object && (!attrs.offset_known_p || !attrs.size_known_p))
- {
- attrs.expr = NULL_TREE;
- attrs.alias = 0;
- }
-
/* Compute the new values of the memory attributes due to this adjustment.
We add the offsets and update the alignment. */
if (attrs.offset_known_p)
{
attrs.offset += offset;
-
- /* Drop the object if the new left end is not within its bounds. */
- if (adjust_object && maybe_lt (attrs.offset, 0))
- {
- attrs.expr = NULL_TREE;
- attrs.alias = 0;
- }
}
/* Compute the new alignment by taking the MIN of the alignment and the
@@ -2445,18 +2430,11 @@ adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
if (maybe_ne (size, 0))
{
- /* Drop the object if the new right end is not within its bounds. */
- if (adjust_object && maybe_gt (offset + size, attrs.size))
- {
- attrs.expr = NULL_TREE;
- attrs.alias = 0;
- }
attrs.size_known_p = true;
attrs.size = size;
}
else if (attrs.size_known_p)
{
- gcc_assert (!adjust_object);
attrs.size -= offset;
/* ??? The store_by_pieces machinery generates negative sizes,
so don't assert for that here. */
@@ -2477,7 +2455,7 @@ adjust_automodify_address_1 (rtx memref, machine_mode mode, rtx addr,
poly_int64 offset, int validate)
{
memref = change_address_1 (memref, VOIDmode, addr, validate, false);
- return adjust_address_1 (memref, mode, offset, validate, 0, 0, 0);
+ return adjust_address_1 (memref, mode, offset, validate, 0, 0);
}
/* Return a memory reference like MEMREF, but whose address is changed by
@@ -2561,7 +2539,7 @@ replace_equiv_address_nv (rtx memref, rtx addr, bool inplace)
rtx
widen_memory_access (rtx memref, machine_mode mode, poly_int64 offset)
{
- rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0, 0);
+ rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0);
poly_uint64 size = GET_MODE_SIZE (mode);
/* If there are no changes, just return the original memory reference. */
diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
index 3d6565c8a30..108669f51a1 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -473,27 +473,27 @@ extern rtx change_address (rtx, machine_mode, rtx);
/* Return a memory reference like MEMREF, but with its mode changed
to MODE and its address offset by OFFSET bytes. */
#define adjust_address(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0, 0)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
/* Likewise, but the reference is not required to be valid. */
#define adjust_address_nv(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0, 0)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
/* Return a memory reference like MEMREF, but with its mode changed
to MODE and its address offset by OFFSET bytes. Assume that it's
for a bitfield and conservatively drop the underlying object if we
cannot be sure to stay within its bounds. */
#define adjust_bitfield_address(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, 0)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
/* As for adjust_bitfield_address, but specify that the width of
BLKmode accesses is SIZE bytes. */
#define adjust_bitfield_address_size(MEMREF, MODE, OFFSET, SIZE) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, SIZE)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, SIZE)
/* Likewise, but the reference is not required to be valid. */
#define adjust_bitfield_address_nv(MEMREF, MODE, OFFSET) \
- adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 1, 0)
+ adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
/* Return a memory reference like MEMREF, but with its mode changed
to MODE and its address changed to ADDR, which is assumed to be
@@ -506,7 +506,7 @@ extern rtx change_address (rtx, machine_mode, rtx);
adjust_automodify_address_1 (MEMREF, MODE, ADDR, OFFSET, 0)
extern rtx adjust_address_1 (rtx, machine_mode, poly_int64, int, int,
- int, poly_int64);
+ poly_int64);
extern rtx adjust_automodify_address_1 (rtx, machine_mode, rtx,
poly_int64, int);
diff --git a/gcc/testsuite/gcc.target/arc/uncached-9.c b/gcc/testsuite/gcc.target/arc/uncached-9.c
new file mode 100644
index 00000000000..44b8a0ed7f3
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arc/uncached-9.c
@@ -0,0 +1,19 @@
+/* { dg-do compile } */
+
+#include <stddef.h>
+
+typedef volatile struct {
+ uint32_t a1;
+ uint32_t a2;
+ uint32_t a3;
+ uint32_t a4;
+} __attribute__((packed,uncached)) my_type_t;
+
+uint32_t foo (my_type_t *p)
+{
+ p->a3 = p->a2;
+ return p->a4;
+}
+
+/* { dg-final { scan-assembler-times "stb\.di" 4 } } */
+/* { dg-final { scan-assembler-times "ldb\.di" 12 } } */
--
2.21.0 (Apple Git-122)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [FIX] Remove object adjustment to preserve object attributes
2020-08-15 22:53 [PATCH] [FIX] Remove object adjustment to preserve object attributes Petro Karashchenko
@ 2020-08-20 8:01 ` Richard Sandiford
2020-08-20 9:07 ` Petro Karashchenko
0 siblings, 1 reply; 4+ messages in thread
From: Richard Sandiford @ 2020-08-20 8:01 UTC (permalink / raw)
To: Petro Karashchenko via Gcc-patches; +Cc: Petro Karashchenko
Petro Karashchenko via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> for bitfield MEMREFs
>
> [FIX] Propagate uncached type attributes to unaligned/packed types
This doesn't look safe in general. The current code was added
to avoid wrong-code problems for accesses that step outside the
bounds of the original MEM_EXPR.
Could you explain in more detail what's going wrong in the testcase?
It wasn't obvious why the code you're removing would trigger for that test.
Thanks,
Richard
>
> [ARC] Update tests
>
> gcc/
> xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
>
> * emit-rtl.c (adjust_address_1): Do not drop the object
> if the new memory reference is outside the underlying
> object to preserve object attributes that are needed
> by some backend implementations. Remove adjust_object
> parameter as it is not used anymore.
> (adjust_automodify_address_1): Adjust according to
> new adjust_address_1 prototype.
> (replace_equiv_address_nv): Likewise.
> * gcc/emit-rtl.h (adjust_address): Adjust according to
> new adjust_address_1 prototype.
> (adjust_address_nv): Likewise.
> (adjust_bitfield_address): Likewise.
> (adjust_bitfield_address_size): Likewise.
> (adjust_bitfield_address_nv): Likewise.
>
> testsuite/
> xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
>
> * gcc.target/arc/uncached-9.c: New file.
>
> Problem description:
> __attribute__((uncached)) is dropped for other than the first member of
> "packed" or "unaligned" types.
>
> Tests:
> Tested with ARC600 bases ASIC. The correct code is generated.
>
> From 1f8a824f8ed8c1452f2bdfc513716c63d5d12545 Mon Sep 17 00:00:00 2001
> From: Petro Karashchenko <petro.karashchenko@gmail.com>
> Date: Sun, 16 Aug 2020 01:10:11 +0300
> Subject: [PATCH] [FIX] Remove object adjustment to preserve object attributes
> for bitfield MEMREFs
>
> [FIX] Propagate uncached type attributes to unaligned/packed types
>
> [ARC] Update tests
>
> gcc/
> xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
>
> * emit-rtl.c (adjust_address_1): Do not drop the object
> if the new memory reference is outside the underlying
> object to preserve object attributes that are needed
> by some backend implementations. Remove adjust_object
> parameter as it is not used anymore.
> (adjust_automodify_address_1): Adjust according to
> new adjust_address_1 prototype.
> (replace_equiv_address_nv): Likewise.
> * gcc/emit-rtl.h (adjust_address): Adjust according to
> new adjust_address_1 prototype.
> (adjust_address_nv): Likewise.
> (adjust_bitfield_address): Likewise.
> (adjust_bitfield_address_size): Likewise.
> (adjust_bitfield_address_nv): Likewise.
>
> testsuite/
> xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
>
> * gcc.target/arc/uncached-9.c: New file.
>
> Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
> ---
> gcc/emit-rtl.c | 28 +++--------------------
> gcc/emit-rtl.h | 12 +++++-----
> gcc/testsuite/gcc.target/arc/uncached-9.c | 19 +++++++++++++++
> 3 files changed, 28 insertions(+), 31 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/arc/uncached-9.c
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index f9b0e9714d9..c0e2ead907b 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -2337,8 +2337,7 @@ change_address (rtx memref, machine_mode mode, rtx addr)
>
> rtx
> adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
> - int validate, int adjust_address, int adjust_object,
> - poly_int64 size)
> + int validate, int adjust_address, poly_int64 size)
> {
> rtx addr = XEXP (memref, 0);
> rtx new_rtx;
> @@ -2413,25 +2412,11 @@ adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
> if (new_rtx == memref && maybe_ne (offset, 0))
> new_rtx = copy_rtx (new_rtx);
>
> - /* Conservatively drop the object if we don't know where we start from. */
> - if (adjust_object && (!attrs.offset_known_p || !attrs.size_known_p))
> - {
> - attrs.expr = NULL_TREE;
> - attrs.alias = 0;
> - }
> -
> /* Compute the new values of the memory attributes due to this adjustment.
> We add the offsets and update the alignment. */
> if (attrs.offset_known_p)
> {
> attrs.offset += offset;
> -
> - /* Drop the object if the new left end is not within its bounds. */
> - if (adjust_object && maybe_lt (attrs.offset, 0))
> - {
> - attrs.expr = NULL_TREE;
> - attrs.alias = 0;
> - }
> }
>
> /* Compute the new alignment by taking the MIN of the alignment and the
> @@ -2445,18 +2430,11 @@ adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
>
> if (maybe_ne (size, 0))
> {
> - /* Drop the object if the new right end is not within its bounds. */
> - if (adjust_object && maybe_gt (offset + size, attrs.size))
> - {
> - attrs.expr = NULL_TREE;
> - attrs.alias = 0;
> - }
> attrs.size_known_p = true;
> attrs.size = size;
> }
> else if (attrs.size_known_p)
> {
> - gcc_assert (!adjust_object);
> attrs.size -= offset;
> /* ??? The store_by_pieces machinery generates negative sizes,
> so don't assert for that here. */
> @@ -2477,7 +2455,7 @@ adjust_automodify_address_1 (rtx memref, machine_mode mode, rtx addr,
> poly_int64 offset, int validate)
> {
> memref = change_address_1 (memref, VOIDmode, addr, validate, false);
> - return adjust_address_1 (memref, mode, offset, validate, 0, 0, 0);
> + return adjust_address_1 (memref, mode, offset, validate, 0, 0);
> }
>
> /* Return a memory reference like MEMREF, but whose address is changed by
> @@ -2561,7 +2539,7 @@ replace_equiv_address_nv (rtx memref, rtx addr, bool inplace)
> rtx
> widen_memory_access (rtx memref, machine_mode mode, poly_int64 offset)
> {
> - rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0, 0);
> + rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0);
> poly_uint64 size = GET_MODE_SIZE (mode);
>
> /* If there are no changes, just return the original memory reference. */
> diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
> index 3d6565c8a30..108669f51a1 100644
> --- a/gcc/emit-rtl.h
> +++ b/gcc/emit-rtl.h
> @@ -473,27 +473,27 @@ extern rtx change_address (rtx, machine_mode, rtx);
> /* Return a memory reference like MEMREF, but with its mode changed
> to MODE and its address offset by OFFSET bytes. */
> #define adjust_address(MEMREF, MODE, OFFSET) \
> - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0, 0)
> + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
>
> /* Likewise, but the reference is not required to be valid. */
> #define adjust_address_nv(MEMREF, MODE, OFFSET) \
> - adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0, 0)
> + adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
>
> /* Return a memory reference like MEMREF, but with its mode changed
> to MODE and its address offset by OFFSET bytes. Assume that it's
> for a bitfield and conservatively drop the underlying object if we
> cannot be sure to stay within its bounds. */
> #define adjust_bitfield_address(MEMREF, MODE, OFFSET) \
> - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, 0)
> + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
>
> /* As for adjust_bitfield_address, but specify that the width of
> BLKmode accesses is SIZE bytes. */
> #define adjust_bitfield_address_size(MEMREF, MODE, OFFSET, SIZE) \
> - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, SIZE)
> + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, SIZE)
>
> /* Likewise, but the reference is not required to be valid. */
> #define adjust_bitfield_address_nv(MEMREF, MODE, OFFSET) \
> - adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 1, 0)
> + adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
>
> /* Return a memory reference like MEMREF, but with its mode changed
> to MODE and its address changed to ADDR, which is assumed to be
> @@ -506,7 +506,7 @@ extern rtx change_address (rtx, machine_mode, rtx);
> adjust_automodify_address_1 (MEMREF, MODE, ADDR, OFFSET, 0)
>
> extern rtx adjust_address_1 (rtx, machine_mode, poly_int64, int, int,
> - int, poly_int64);
> + poly_int64);
> extern rtx adjust_automodify_address_1 (rtx, machine_mode, rtx,
> poly_int64, int);
>
> diff --git a/gcc/testsuite/gcc.target/arc/uncached-9.c b/gcc/testsuite/gcc.target/arc/uncached-9.c
> new file mode 100644
> index 00000000000..44b8a0ed7f3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arc/uncached-9.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +
> +#include <stddef.h>
> +
> +typedef volatile struct {
> + uint32_t a1;
> + uint32_t a2;
> + uint32_t a3;
> + uint32_t a4;
> +} __attribute__((packed,uncached)) my_type_t;
> +
> +uint32_t foo (my_type_t *p)
> +{
> + p->a3 = p->a2;
> + return p->a4;
> +}
> +
> +/* { dg-final { scan-assembler-times "stb\.di" 4 } } */
> +/* { dg-final { scan-assembler-times "ldb\.di" 12 } } */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [FIX] Remove object adjustment to preserve object attributes
2020-08-20 8:01 ` Richard Sandiford
@ 2020-08-20 9:07 ` Petro Karashchenko
2020-08-24 9:21 ` Richard Biener
0 siblings, 1 reply; 4+ messages in thread
From: Petro Karashchenko @ 2020-08-20 9:07 UTC (permalink / raw)
To: Petro Karashchenko via Gcc-patches, Petro Karashchenko,
richard.sandiford
Hello Ricard!
Thank you very much for your reply.
The case is that currently the "uncached" attribute is used to generate
special "cache bypass" instructions instead of regular one by ARC backend.
That decision is made based on the result of "lookup_attribute()" call for
a specified MEM_REF. For example
if (TREE_CODE (addr) == MEM_REF)
{
attrs = TYPE_ATTRIBUTES (TREE_TYPE (TREE_OPERAND (addr, 0)));
if (lookup_attribute ("uncached", attrs))
return true;
attrs = TYPE_ATTRIBUTES (TREE_TYPE (TREE_OPERAND (addr, 1)));
if (lookup_attribute ("uncached", attrs))
return true;
}
That works pretty well for aligned types, but for unaligned/packed types
(also for example for array -X access) the expression is dropped. In my
test example with current code the "a1" will still inherit "uncached"
attribute because "adjust_object" will not be done, but "a2", "a3" and "a4"
will drop expression, hence "uncached" attribute can't be accessed.
Does that make sense?
Maybe we can clone/copy expression instead of dropping to preserve
"uncached" attribute?
Best Regards,
Petro Karashchenko
чт, 20 серп. 2020 о 11:01 Richard Sandiford <richard.sandiford@arm.com>
пише:
> Petro Karashchenko via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > for bitfield MEMREFs
> >
> > [FIX] Propagate uncached type attributes to unaligned/packed types
>
> This doesn't look safe in general. The current code was added
> to avoid wrong-code problems for accesses that step outside the
> bounds of the original MEM_EXPR.
>
> Could you explain in more detail what's going wrong in the testcase?
> It wasn't obvious why the code you're removing would trigger for that test.
>
> Thanks,
> Richard
>
> >
> > [ARC] Update tests
> >
> > gcc/
> > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> >
> > * emit-rtl.c (adjust_address_1): Do not drop the object
> > if the new memory reference is outside the underlying
> > object to preserve object attributes that are needed
> > by some backend implementations. Remove adjust_object
> > parameter as it is not used anymore.
> > (adjust_automodify_address_1): Adjust according to
> > new adjust_address_1 prototype.
> > (replace_equiv_address_nv): Likewise.
> > * gcc/emit-rtl.h (adjust_address): Adjust according to
> > new adjust_address_1 prototype.
> > (adjust_address_nv): Likewise.
> > (adjust_bitfield_address): Likewise.
> > (adjust_bitfield_address_size): Likewise.
> > (adjust_bitfield_address_nv): Likewise.
> >
> > testsuite/
> > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> >
> > * gcc.target/arc/uncached-9.c: New file.
> >
> > Problem description:
> > __attribute__((uncached)) is dropped for other than the first member of
> > "packed" or "unaligned" types.
> >
> > Tests:
> > Tested with ARC600 bases ASIC. The correct code is generated.
> >
> > From 1f8a824f8ed8c1452f2bdfc513716c63d5d12545 Mon Sep 17 00:00:00 2001
> > From: Petro Karashchenko <petro.karashchenko@gmail.com>
> > Date: Sun, 16 Aug 2020 01:10:11 +0300
> > Subject: [PATCH] [FIX] Remove object adjustment to preserve object
> attributes
> > for bitfield MEMREFs
> >
> > [FIX] Propagate uncached type attributes to unaligned/packed types
> >
> > [ARC] Update tests
> >
> > gcc/
> > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> >
> > * emit-rtl.c (adjust_address_1): Do not drop the object
> > if the new memory reference is outside the underlying
> > object to preserve object attributes that are needed
> > by some backend implementations. Remove adjust_object
> > parameter as it is not used anymore.
> > (adjust_automodify_address_1): Adjust according to
> > new adjust_address_1 prototype.
> > (replace_equiv_address_nv): Likewise.
> > * gcc/emit-rtl.h (adjust_address): Adjust according to
> > new adjust_address_1 prototype.
> > (adjust_address_nv): Likewise.
> > (adjust_bitfield_address): Likewise.
> > (adjust_bitfield_address_size): Likewise.
> > (adjust_bitfield_address_nv): Likewise.
> >
> > testsuite/
> > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> >
> > * gcc.target/arc/uncached-9.c: New file.
> >
> > Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
> > ---
> > gcc/emit-rtl.c | 28 +++--------------------
> > gcc/emit-rtl.h | 12 +++++-----
> > gcc/testsuite/gcc.target/arc/uncached-9.c | 19 +++++++++++++++
> > 3 files changed, 28 insertions(+), 31 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/arc/uncached-9.c
> >
> > diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> > index f9b0e9714d9..c0e2ead907b 100644
> > --- a/gcc/emit-rtl.c
> > +++ b/gcc/emit-rtl.c
> > @@ -2337,8 +2337,7 @@ change_address (rtx memref, machine_mode mode, rtx
> addr)
> >
> > rtx
> > adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
> > - int validate, int adjust_address, int adjust_object,
> > - poly_int64 size)
> > + int validate, int adjust_address, poly_int64 size)
> > {
> > rtx addr = XEXP (memref, 0);
> > rtx new_rtx;
> > @@ -2413,25 +2412,11 @@ adjust_address_1 (rtx memref, machine_mode mode,
> poly_int64 offset,
> > if (new_rtx == memref && maybe_ne (offset, 0))
> > new_rtx = copy_rtx (new_rtx);
> >
> > - /* Conservatively drop the object if we don't know where we start
> from. */
> > - if (adjust_object && (!attrs.offset_known_p || !attrs.size_known_p))
> > - {
> > - attrs.expr = NULL_TREE;
> > - attrs.alias = 0;
> > - }
> > -
> > /* Compute the new values of the memory attributes due to this
> adjustment.
> > We add the offsets and update the alignment. */
> > if (attrs.offset_known_p)
> > {
> > attrs.offset += offset;
> > -
> > - /* Drop the object if the new left end is not within its bounds.
> */
> > - if (adjust_object && maybe_lt (attrs.offset, 0))
> > - {
> > - attrs.expr = NULL_TREE;
> > - attrs.alias = 0;
> > - }
> > }
> >
> > /* Compute the new alignment by taking the MIN of the alignment and
> the
> > @@ -2445,18 +2430,11 @@ adjust_address_1 (rtx memref, machine_mode mode,
> poly_int64 offset,
> >
> > if (maybe_ne (size, 0))
> > {
> > - /* Drop the object if the new right end is not within its
> bounds. */
> > - if (adjust_object && maybe_gt (offset + size, attrs.size))
> > - {
> > - attrs.expr = NULL_TREE;
> > - attrs.alias = 0;
> > - }
> > attrs.size_known_p = true;
> > attrs.size = size;
> > }
> > else if (attrs.size_known_p)
> > {
> > - gcc_assert (!adjust_object);
> > attrs.size -= offset;
> > /* ??? The store_by_pieces machinery generates negative sizes,
> > so don't assert for that here. */
> > @@ -2477,7 +2455,7 @@ adjust_automodify_address_1 (rtx memref,
> machine_mode mode, rtx addr,
> > poly_int64 offset, int validate)
> > {
> > memref = change_address_1 (memref, VOIDmode, addr, validate, false);
> > - return adjust_address_1 (memref, mode, offset, validate, 0, 0, 0);
> > + return adjust_address_1 (memref, mode, offset, validate, 0, 0);
> > }
> >
> > /* Return a memory reference like MEMREF, but whose address is changed
> by
> > @@ -2561,7 +2539,7 @@ replace_equiv_address_nv (rtx memref, rtx addr,
> bool inplace)
> > rtx
> > widen_memory_access (rtx memref, machine_mode mode, poly_int64 offset)
> > {
> > - rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0, 0);
> > + rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0);
> > poly_uint64 size = GET_MODE_SIZE (mode);
> >
> > /* If there are no changes, just return the original memory
> reference. */
> > diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
> > index 3d6565c8a30..108669f51a1 100644
> > --- a/gcc/emit-rtl.h
> > +++ b/gcc/emit-rtl.h
> > @@ -473,27 +473,27 @@ extern rtx change_address (rtx, machine_mode, rtx);
> > /* Return a memory reference like MEMREF, but with its mode changed
> > to MODE and its address offset by OFFSET bytes. */
> > #define adjust_address(MEMREF, MODE, OFFSET) \
> > - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0, 0)
> > + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
> >
> > /* Likewise, but the reference is not required to be valid. */
> > #define adjust_address_nv(MEMREF, MODE, OFFSET) \
> > - adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0, 0)
> > + adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
> >
> > /* Return a memory reference like MEMREF, but with its mode changed
> > to MODE and its address offset by OFFSET bytes. Assume that it's
> > for a bitfield and conservatively drop the underlying object if we
> > cannot be sure to stay within its bounds. */
> > #define adjust_bitfield_address(MEMREF, MODE, OFFSET) \
> > - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, 0)
> > + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
> >
> > /* As for adjust_bitfield_address, but specify that the width of
> > BLKmode accesses is SIZE bytes. */
> > #define adjust_bitfield_address_size(MEMREF, MODE, OFFSET, SIZE) \
> > - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, SIZE)
> > + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, SIZE)
> >
> > /* Likewise, but the reference is not required to be valid. */
> > #define adjust_bitfield_address_nv(MEMREF, MODE, OFFSET) \
> > - adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 1, 0)
> > + adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
> >
> > /* Return a memory reference like MEMREF, but with its mode changed
> > to MODE and its address changed to ADDR, which is assumed to be
> > @@ -506,7 +506,7 @@ extern rtx change_address (rtx, machine_mode, rtx);
> > adjust_automodify_address_1 (MEMREF, MODE, ADDR, OFFSET, 0)
> >
> > extern rtx adjust_address_1 (rtx, machine_mode, poly_int64, int, int,
> > - int, poly_int64);
> > + poly_int64);
> > extern rtx adjust_automodify_address_1 (rtx, machine_mode, rtx,
> > poly_int64, int);
> >
> > diff --git a/gcc/testsuite/gcc.target/arc/uncached-9.c
> b/gcc/testsuite/gcc.target/arc/uncached-9.c
> > new file mode 100644
> > index 00000000000..44b8a0ed7f3
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arc/uncached-9.c
> > @@ -0,0 +1,19 @@
> > +/* { dg-do compile } */
> > +
> > +#include <stddef.h>
> > +
> > +typedef volatile struct {
> > + uint32_t a1;
> > + uint32_t a2;
> > + uint32_t a3;
> > + uint32_t a4;
> > +} __attribute__((packed,uncached)) my_type_t;
> > +
> > +uint32_t foo (my_type_t *p)
> > +{
> > + p->a3 = p->a2;
> > + return p->a4;
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "stb\.di" 4 } } */
> > +/* { dg-final { scan-assembler-times "ldb\.di" 12 } } */
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] [FIX] Remove object adjustment to preserve object attributes
2020-08-20 9:07 ` Petro Karashchenko
@ 2020-08-24 9:21 ` Richard Biener
0 siblings, 0 replies; 4+ messages in thread
From: Richard Biener @ 2020-08-24 9:21 UTC (permalink / raw)
To: Petro Karashchenko; +Cc: Petro Karashchenko via Gcc-patches, Richard Sandiford
On Thu, Aug 20, 2020 at 11:09 AM Petro Karashchenko via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hello Ricard!
>
> Thank you very much for your reply.
> The case is that currently the "uncached" attribute is used to generate
> special "cache bypass" instructions instead of regular one by ARC backend.
> That decision is made based on the result of "lookup_attribute()" call for
> a specified MEM_REF. For example
> if (TREE_CODE (addr) == MEM_REF)
> {
> attrs = TYPE_ATTRIBUTES (TREE_TYPE (TREE_OPERAND (addr, 0)));
Just a note - this looks very unsafe since the type of the address operand to
the MEM_REF is quite arbitrary.
> if (lookup_attribute ("uncached", attrs))
> return true;
> attrs = TYPE_ATTRIBUTES (TREE_TYPE (TREE_OPERAND (addr, 1)));
> if (lookup_attribute ("uncached", attrs))
> return true;
> }
>
> That works pretty well for aligned types, but for unaligned/packed types
> (also for example for array -X access) the expression is dropped. In my
> test example with current code the "a1" will still inherit "uncached"
> attribute because "adjust_object" will not be done, but "a2", "a3" and "a4"
> will drop expression, hence "uncached" attribute can't be accessed.
>
> Does that make sense?
Hmm, the MEM_EXPR is used for optimization only and may _not_ be
the only way to carry information required for correctness. That said,
dropping mem_attrs must be always correct.
This means you have to maintain the 'uncached' effect elsewhere.
Richard.
> Maybe we can clone/copy expression instead of dropping to preserve
> "uncached" attribute?
>
> Best Regards,
> Petro Karashchenko
>
> чт, 20 серп. 2020 о 11:01 Richard Sandiford <richard.sandiford@arm.com>
> пише:
>
> > Petro Karashchenko via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > > for bitfield MEMREFs
> > >
> > > [FIX] Propagate uncached type attributes to unaligned/packed types
> >
> > This doesn't look safe in general. The current code was added
> > to avoid wrong-code problems for accesses that step outside the
> > bounds of the original MEM_EXPR.
> >
> > Could you explain in more detail what's going wrong in the testcase?
> > It wasn't obvious why the code you're removing would trigger for that test.
> >
> > Thanks,
> > Richard
> >
> > >
> > > [ARC] Update tests
> > >
> > > gcc/
> > > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> > >
> > > * emit-rtl.c (adjust_address_1): Do not drop the object
> > > if the new memory reference is outside the underlying
> > > object to preserve object attributes that are needed
> > > by some backend implementations. Remove adjust_object
> > > parameter as it is not used anymore.
> > > (adjust_automodify_address_1): Adjust according to
> > > new adjust_address_1 prototype.
> > > (replace_equiv_address_nv): Likewise.
> > > * gcc/emit-rtl.h (adjust_address): Adjust according to
> > > new adjust_address_1 prototype.
> > > (adjust_address_nv): Likewise.
> > > (adjust_bitfield_address): Likewise.
> > > (adjust_bitfield_address_size): Likewise.
> > > (adjust_bitfield_address_nv): Likewise.
> > >
> > > testsuite/
> > > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> > >
> > > * gcc.target/arc/uncached-9.c: New file.
> > >
> > > Problem description:
> > > __attribute__((uncached)) is dropped for other than the first member of
> > > "packed" or "unaligned" types.
> > >
> > > Tests:
> > > Tested with ARC600 bases ASIC. The correct code is generated.
> > >
> > > From 1f8a824f8ed8c1452f2bdfc513716c63d5d12545 Mon Sep 17 00:00:00 2001
> > > From: Petro Karashchenko <petro.karashchenko@gmail.com>
> > > Date: Sun, 16 Aug 2020 01:10:11 +0300
> > > Subject: [PATCH] [FIX] Remove object adjustment to preserve object
> > attributes
> > > for bitfield MEMREFs
> > >
> > > [FIX] Propagate uncached type attributes to unaligned/packed types
> > >
> > > [ARC] Update tests
> > >
> > > gcc/
> > > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> > >
> > > * emit-rtl.c (adjust_address_1): Do not drop the object
> > > if the new memory reference is outside the underlying
> > > object to preserve object attributes that are needed
> > > by some backend implementations. Remove adjust_object
> > > parameter as it is not used anymore.
> > > (adjust_automodify_address_1): Adjust according to
> > > new adjust_address_1 prototype.
> > > (replace_equiv_address_nv): Likewise.
> > > * gcc/emit-rtl.h (adjust_address): Adjust according to
> > > new adjust_address_1 prototype.
> > > (adjust_address_nv): Likewise.
> > > (adjust_bitfield_address): Likewise.
> > > (adjust_bitfield_address_size): Likewise.
> > > (adjust_bitfield_address_nv): Likewise.
> > >
> > > testsuite/
> > > xxxx-xx-xx Petro Karashchenko <petro.karashchenko@gmail.com>
> > >
> > > * gcc.target/arc/uncached-9.c: New file.
> > >
> > > Signed-off-by: Petro Karashchenko <petro.karashchenko@gmail.com>
> > > ---
> > > gcc/emit-rtl.c | 28 +++--------------------
> > > gcc/emit-rtl.h | 12 +++++-----
> > > gcc/testsuite/gcc.target/arc/uncached-9.c | 19 +++++++++++++++
> > > 3 files changed, 28 insertions(+), 31 deletions(-)
> > > create mode 100644 gcc/testsuite/gcc.target/arc/uncached-9.c
> > >
> > > diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> > > index f9b0e9714d9..c0e2ead907b 100644
> > > --- a/gcc/emit-rtl.c
> > > +++ b/gcc/emit-rtl.c
> > > @@ -2337,8 +2337,7 @@ change_address (rtx memref, machine_mode mode, rtx
> > addr)
> > >
> > > rtx
> > > adjust_address_1 (rtx memref, machine_mode mode, poly_int64 offset,
> > > - int validate, int adjust_address, int adjust_object,
> > > - poly_int64 size)
> > > + int validate, int adjust_address, poly_int64 size)
> > > {
> > > rtx addr = XEXP (memref, 0);
> > > rtx new_rtx;
> > > @@ -2413,25 +2412,11 @@ adjust_address_1 (rtx memref, machine_mode mode,
> > poly_int64 offset,
> > > if (new_rtx == memref && maybe_ne (offset, 0))
> > > new_rtx = copy_rtx (new_rtx);
> > >
> > > - /* Conservatively drop the object if we don't know where we start
> > from. */
> > > - if (adjust_object && (!attrs.offset_known_p || !attrs.size_known_p))
> > > - {
> > > - attrs.expr = NULL_TREE;
> > > - attrs.alias = 0;
> > > - }
> > > -
> > > /* Compute the new values of the memory attributes due to this
> > adjustment.
> > > We add the offsets and update the alignment. */
> > > if (attrs.offset_known_p)
> > > {
> > > attrs.offset += offset;
> > > -
> > > - /* Drop the object if the new left end is not within its bounds.
> > */
> > > - if (adjust_object && maybe_lt (attrs.offset, 0))
> > > - {
> > > - attrs.expr = NULL_TREE;
> > > - attrs.alias = 0;
> > > - }
> > > }
> > >
> > > /* Compute the new alignment by taking the MIN of the alignment and
> > the
> > > @@ -2445,18 +2430,11 @@ adjust_address_1 (rtx memref, machine_mode mode,
> > poly_int64 offset,
> > >
> > > if (maybe_ne (size, 0))
> > > {
> > > - /* Drop the object if the new right end is not within its
> > bounds. */
> > > - if (adjust_object && maybe_gt (offset + size, attrs.size))
> > > - {
> > > - attrs.expr = NULL_TREE;
> > > - attrs.alias = 0;
> > > - }
> > > attrs.size_known_p = true;
> > > attrs.size = size;
> > > }
> > > else if (attrs.size_known_p)
> > > {
> > > - gcc_assert (!adjust_object);
> > > attrs.size -= offset;
> > > /* ??? The store_by_pieces machinery generates negative sizes,
> > > so don't assert for that here. */
> > > @@ -2477,7 +2455,7 @@ adjust_automodify_address_1 (rtx memref,
> > machine_mode mode, rtx addr,
> > > poly_int64 offset, int validate)
> > > {
> > > memref = change_address_1 (memref, VOIDmode, addr, validate, false);
> > > - return adjust_address_1 (memref, mode, offset, validate, 0, 0, 0);
> > > + return adjust_address_1 (memref, mode, offset, validate, 0, 0);
> > > }
> > >
> > > /* Return a memory reference like MEMREF, but whose address is changed
> > by
> > > @@ -2561,7 +2539,7 @@ replace_equiv_address_nv (rtx memref, rtx addr,
> > bool inplace)
> > > rtx
> > > widen_memory_access (rtx memref, machine_mode mode, poly_int64 offset)
> > > {
> > > - rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0, 0);
> > > + rtx new_rtx = adjust_address_1 (memref, mode, offset, 1, 1, 0);
> > > poly_uint64 size = GET_MODE_SIZE (mode);
> > >
> > > /* If there are no changes, just return the original memory
> > reference. */
> > > diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
> > > index 3d6565c8a30..108669f51a1 100644
> > > --- a/gcc/emit-rtl.h
> > > +++ b/gcc/emit-rtl.h
> > > @@ -473,27 +473,27 @@ extern rtx change_address (rtx, machine_mode, rtx);
> > > /* Return a memory reference like MEMREF, but with its mode changed
> > > to MODE and its address offset by OFFSET bytes. */
> > > #define adjust_address(MEMREF, MODE, OFFSET) \
> > > - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0, 0)
> > > + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
> > >
> > > /* Likewise, but the reference is not required to be valid. */
> > > #define adjust_address_nv(MEMREF, MODE, OFFSET) \
> > > - adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0, 0)
> > > + adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
> > >
> > > /* Return a memory reference like MEMREF, but with its mode changed
> > > to MODE and its address offset by OFFSET bytes. Assume that it's
> > > for a bitfield and conservatively drop the underlying object if we
> > > cannot be sure to stay within its bounds. */
> > > #define adjust_bitfield_address(MEMREF, MODE, OFFSET) \
> > > - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, 0)
> > > + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 0)
> > >
> > > /* As for adjust_bitfield_address, but specify that the width of
> > > BLKmode accesses is SIZE bytes. */
> > > #define adjust_bitfield_address_size(MEMREF, MODE, OFFSET, SIZE) \
> > > - adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, 1, SIZE)
> > > + adjust_address_1 (MEMREF, MODE, OFFSET, 1, 1, SIZE)
> > >
> > > /* Likewise, but the reference is not required to be valid. */
> > > #define adjust_bitfield_address_nv(MEMREF, MODE, OFFSET) \
> > > - adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 1, 0)
> > > + adjust_address_1 (MEMREF, MODE, OFFSET, 0, 1, 0)
> > >
> > > /* Return a memory reference like MEMREF, but with its mode changed
> > > to MODE and its address changed to ADDR, which is assumed to be
> > > @@ -506,7 +506,7 @@ extern rtx change_address (rtx, machine_mode, rtx);
> > > adjust_automodify_address_1 (MEMREF, MODE, ADDR, OFFSET, 0)
> > >
> > > extern rtx adjust_address_1 (rtx, machine_mode, poly_int64, int, int,
> > > - int, poly_int64);
> > > + poly_int64);
> > > extern rtx adjust_automodify_address_1 (rtx, machine_mode, rtx,
> > > poly_int64, int);
> > >
> > > diff --git a/gcc/testsuite/gcc.target/arc/uncached-9.c
> > b/gcc/testsuite/gcc.target/arc/uncached-9.c
> > > new file mode 100644
> > > index 00000000000..44b8a0ed7f3
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/arc/uncached-9.c
> > > @@ -0,0 +1,19 @@
> > > +/* { dg-do compile } */
> > > +
> > > +#include <stddef.h>
> > > +
> > > +typedef volatile struct {
> > > + uint32_t a1;
> > > + uint32_t a2;
> > > + uint32_t a3;
> > > + uint32_t a4;
> > > +} __attribute__((packed,uncached)) my_type_t;
> > > +
> > > +uint32_t foo (my_type_t *p)
> > > +{
> > > + p->a3 = p->a2;
> > > + return p->a4;
> > > +}
> > > +
> > > +/* { dg-final { scan-assembler-times "stb\.di" 4 } } */
> > > +/* { dg-final { scan-assembler-times "ldb\.di" 12 } } */
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-24 9:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-15 22:53 [PATCH] [FIX] Remove object adjustment to preserve object attributes Petro Karashchenko
2020-08-20 8:01 ` Richard Sandiford
2020-08-20 9:07 ` Petro Karashchenko
2020-08-24 9:21 ` Richard Biener
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).