On Wed, 2014-06-25 at 14:39 -0600, Jeff Law wrote:
> On 06/25/14 03:36, Richard Sandiford wrote:
> >
> > 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:
> Historical coding style :( It's particularly bad in RTL land, but you
> see the same problems in places like fold-const.
>
>
> >
> > 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 "()".)
> There was some talk of revamping the rules of parens for member function
> calls. I don't recall what the final outcome was.
>
> I prefer the latter, but I tend to worry about making David's patches
> even more invasive than they already are :-)
:)
Yeah, that's probably my primary concern here. The patch kit is going
to be big (currently at 133 patches [1]), and so I want something that
we can sanely keep track of, that is easily reviewable, and will be as
easy as possible to merge.
i.e. I don't want to get bogged down in a big revamp of the rest of the
RTL interface if I can help it.
I'm attempting to focus on splitting out insns from expressions. As
mentioned before, in the v1 of this patch kit I also introduced rtx
subclasses for various core types like INSN_LIST, SEQUENCE, etc - but in
this iteration I'm attempting to do it without those - not yet sure if
it's possible.
If it's desirable to actually make insns be a separate class, I'm
considering the goal of making the attributes of insns become actual
fields, something like:
class rtx_insn /* we can bikeshed over the name */
{
public:
rtx_insn *m_prev;
rtx_insn *m_next;
int m_uid;
};
#define PREV_INSN(INSN) ((INSN)->m_prev)
#define NEXT_INSN(INSN) ((INSN)->m_next)
#define INSN_UID(INSN) ((INSN)->m_uid)
/* or we could convert them to functions returning
references, I guess */
...etc for the subclasses, giving something that gdb can print sanely,
and, I hope, more amenable to optimization when gcc itself is compiled.
But even if we don't get there and simply keep insns as subclasses of
rtx, I think that having insn-handling code marked as such in the
type-system is a win from a readability standpoint.
Either way, I think this should be much more "grokkable" by new
contributors. My own experience is that RTL was the aspect of GCC I had
most difficulty with when I was a newbie - hence my motivation to "drain
this swamp".
Hope these ideas sound sane
Dave
[1] FWIW the latest version of the patches is here:
http://dmalcolm.fedorapeople.org/gcc/patch-backups/rtx-classes/v9/
at 133 patches (as before it's relative to r211061). I'm aiming for
lots of *small* patches. Successful bootstrap and regrtest on x86_64;
builds on 187 configurations. Perhaps the biggest change since last
time is that the "scaffolding" phase of the patches introduces a hack
into the generated inns-*.c so that the .md files see rtx_insn * rather
than plain rtx (for "insn" and "curr_insn"), which should allow lots of
target-specific hooks used from .md files to be similarly converted:
http://dmalcolm.fedorapeople.org/gcc/patch-backups/rtx-classes/v9/0036-Use-rtx_insn-internally-within-generated-functions.patch
> > 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.
> Please, no...
>
>
> >
> > 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.
> And probably faster as well since we've CSE'd the memory reference. In
> this specific example it probably doesn't matter, but often there's a
> call somewhere between XEXPs that we'd like to CSE that destroys our
> ability to CSE away memory references.
>
> This kind of problem is prevasive in the RTL analysis/optimizers.
>
