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.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 } } */
> >
