[PATCH] [FIX] Remove object adjustment to preserve object attributes

Richard Biener richard.guenther@gmail.com
Mon Aug 24 09:21:11 GMT 2020


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


More information about the Gcc-patches mailing list