Re: Instructions vs Expressions in the backend (was Re: RFA: Rework FOR_BB_INSNS iterators)

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2014-06-25 at 10:36 +0100, Richard Sandiford wrote:
> Oleg Endo <oleg.endo@t-online.de> writes:
> > Personally, I'd like to see usage of standard STL-like iterator usage.
> > I've proposed something for edge_iterator a while ago, but people don't
> > seem very fond of it.  See also
> > https://gcc.gnu.org/ml/gcc-patches/2013-12/msg01129.html
> >
> > Have you also considered passing the new rtx_* types by value or
> > reference instead of pointer?  A long time ago I've quickly put together
> > a class 'rtxx' which was just a pointer wrapper for the rtx_def*
> > (basically the same as I proposed for edge_iterator).
> > I've converted the SH backend code to use it just to see what it would
> > look like.  The conversion itself was pretty straight forward -- just
> > replace 'rtx' with 'rtxx'.  Appropriate conversion
> > operators/constructors in 'class rtxx' made both interchangeable and
> > allowed co-existence of both and thus step-by-step conversion of the
> > code base.
> > Another advantage of passing around by value/ref is that it allows
> > operator overloading.  One use case could be instead of:
> >
> > if (MEM_P (XEXP (x, 0)))
> >   *total = address_cost (XEXP (XEXP (x, 0), 0),
> >                          GET_MODE (XEXP (x, 0)),
> >                          MEM_ADDR_SPACE (XEXP (x, 0)), true);
> >
> >
> > something like that (overloading operator[]):
> > if (x[0] == rtx_mem::type)
> >   *total = address_cost (x[0][0], x[0].mode (),
> >                          x[0].mem_addr_space (), true);
> >
> > ... where rtx_mem::type would be some type for which 'rtxx' (or whatever
> > the name of the base class is) would provide the according operator
> > ==, != overloads.
> 
> I think this is an example of another problem with gcc coding style:
> that we're far too afraid of temporary variables.  In David's scheme
> I think this would be:
> 
>   if (rtx_mem *mem = as_a <rtx_mem *> (XEXP (x, 0)))
>     *total = address_cost (XEXP (mem, 0), GET_MODE (mem),
>                            MEM_ADDR_SPACE (mem), true);

FWIW you want a dyn_cast<> rather than an as_a<> here, giving:

  if (rtx_mem *mem = dyn_cast <rtx_mem *> (XEXP (x, 0)))
    *total = address_cost (XEXP (mem, 0), GET_MODE (mem),
                           MEM_ADDR_SPACE (mem), true);

> which with members would become:
> 
>   if (rtx_mem *mem = as_a <rtx_mem *> (...))
>     *total = address_cost (mem->address (), mem->mode (), mem->address_space (),
>                            true);

(likewise)

> (although if we go down that route, I hope we can add an exception to the
> formatting rule so that no space should be used before "()".)
> 
> I suppose with the magic values it would be:
> 
>   if (rtx_mem mem = as_a <rtx_mem> (x[0]))
>     *total = address_cost (mem[0], mem.mode (), mem.address_space (), true);

(likewise).

> but I'm not sure that that would really be more readable.

[...snip...; see my other mail for notes on restricting the scope of the
current patch kit to an insn vs expr separation, for the sake of my
sanity :) ]