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.sandif...@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.karashche...@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.karashche...@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.karashche...@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.karashche...@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.karashche...@gmail.com>
> >
> > * gcc.target/arc/uncached-9.c: New file.
> >
> > Signed-off-by: Petro Karashchenko <petro.karashche...@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 } } */
>
