On Wed, 2014-06-25 at 10:36 +0100, Richard Sandiford wrote:
> Oleg Endo <oleg.e...@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);
>
> which with members would become:
>
> if (rtx_mem *mem = as_a <rtx_mem *> (...))
> *total = address_cost (mem->address (), mem->mode (), mem->address_space
> (),
> true);
>
> (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 "()".)
If such an exception is introduced, I can imagine it'd be difficult to
judge on a case by case whether to apply the exceptional rule or not,
since those are just function calls.
But in the end it doesn't matter. It's just a matter of habbit :)
>
> 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);
>
> but I'm not sure that that would really be more readable.
No, essentially it's the same. Whether the current XEXP (x, n) is done
by operator [] or a member function 'xexp (n)' doesn't matter. The
biggest (optical) change is for nested XEXP...
XEXP (XEXP (XEXP (x, a), b), c)
becomes
x.exp (a).exp (b).exp (c)
> FWIW I also did an experiment locally with replacing "rtx *" (rather than
> "rtx") with a "smart" pointer so that we could have four allocation
> areas: permanent, gty, function and temporary, with the pointer
> automatically promoting rtxes to the right allocation area for the
> containing object.
>
> Having a smart pointer made it suprisingly uninvasive but there was
> probably too much C++ magic involved in the handling of XEXP, etc.,
> for the patch to be acceptable. Still, it did noticeably reduce max RSS
> and was a significant compile-time saving for extreme compiles like
> insn-recog.ii. Hope to return to it sometime...
Yep, passing the new rtx classes by value instead by pointer opens other
doors, such as implementing smart pointers internally and what not.
>
> > Another thing is rtx construction in C++ code, which could look like:
> >
> > emit_insn (rtx_insn (rtx_set (rtx_reg (0),
> > rtx_plus (rtx_reg (1), rtx_reg (2)))));
> > emit_insn (rtx_barrier ());
> >
> > Although I'm not sure how often this is needed in current practice,
> > since most of the time rtx instances are created from the .md patterns.
> > Maybe it could be useful for implementing some kind of ad-hoc rtx
> > matching, as it's found in cost calculations, predicates, constrants,
> > e.g.
> >
> > if ((GET_CODE (XEXP (x, 0)) == SMAX || GET_CODE (XEXP (x, 0)) == SMIN)
> > && CONST_INT_P (XEXP (XEXP (x, 0), 1))
> > && REG_P (XEXP (XEXP (x, 0), 0))
> > && CONST_INT_P (XEXP (x, 1)))
> >
> > could become:
> > if (matches (x, rtx_smax (reg_rtx (), const_int (), const_int ()))
> > || matches (x, rtx_smin (reg_rtx (), const_int (), const_int ()))
> >
> > somehow I find that easier to read and write.
>
> It sounds like this would be creating temporary rtxes though, which would
> be too expensive for matching.
Yeah, as I wrote it up there, probably. It depends on the
implementation of those default constructed objects.
> Maybe it would make sense to have a separate
> set of matching objects that only live for the duration of the statement.
> Then you could have matching objects like "range (min, max)" to match a
> range of integers.
Maybe something like this ...
static my_match_rtx = rtx_smax (reg_rtx (), const_int (), const_int ());
if (matches (x, my_match_rtx) ...
> But like you say, I'm not sure whether it would really be a win in the end.
> I think what makes this example so hard to read is again that we refuse
> to put XEXP (x, 0) in a temporary variable and just write it out in full
> 4 times instead.
>
> if ((GET_CODE (op0) == SMAX || GET_CODE (op0) == SMIN)
> && CONST_INT_P (XEXP (op0, 1))
> && REG_P (XEXP (op0, 0))
> && CONST_INT_P (op1))
>
> is a bit more obvious.
In this case yes. On the other hand, that if statement is part of a
bigger function. There are cases, where manual CSE is not so practical,
e.g. when it's an if - else if - else if - else.
Cheers,
Oleg
