This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH 000/236] Introduce rtx subclasses
- From: Trevor Saunders <tsaunders at mozilla dot com>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Thu, 7 Aug 2014 06:29:39 -0400
- Subject: Re: [PATCH 000/236] Introduce rtx subclasses
- Authentication-results: sourceware.org; auth=none
- References: <1407345815-14551-1-git-send-email-dmalcolm at redhat dot com>
On Wed, Aug 06, 2014 at 01:19:39PM -0400, David Malcolm wrote:
> This is the patch series I spoke about at Cauldron in the talk
> "A proposal for typesafe RTL"; slides here:
> They can also be seen at:
> The aim of the patch series is to improve the type-safety and
> readability of the backend by introducing subclasses of rtx (actually
> rtx_def) for *instructions*, and also for EXPR_LIST, INSN_LIST, SEQUENCE.
> That way we can document directly in the code the various places that
> manipulate insn chains vs other kinds of rtx node.
makes sense, thanks for doing this.
> * having a subclass for EXPR_LIST allows for replacing this kind of
> for (x = forced_labels; x; x = XEXP (x, 1))
> if (XEXP (x, 0))
> set_label_offsets (XEXP (x, 0), NULL_RTX, 1);
> with the following, which captures that it's an EXPR_LIST, and makes
> it clearer that we're simply walking a singly-linked list:
> for (rtx_expr_list *x = forced_labels; x; x = x->next ())
> if (x->element ())
> set_label_offsets (x->element (), NULL_RTX, 1);
much nicer, I guess this may help us eventually make these classes stop
being rtxen and so save some memory.
> * there's a NULL_RTX define in rtl.h. In an earlier version of the
> patch I added a NULL_INSN define, but in this version I simply use
> NULL, and I'm in two minds about whether a NULL_INSN is desirable
> (would we have a NULL_FOO for all of the subclasses?). I like having
> a strong distinction between arbitrary RTL nodes vs instructions,
> so maybe there's a case for NULL_INSN, but not for the subclasses?
tbh I don't understand the point of NULL_RTX / TREE. I tend to think
there's no point in multiple names for NULL. After all we don't have a
NULL_INT or NULL_FLOAT, but float * and int * are pretty different.
> * "pointerness" of the types. "rtx" is a typedef to "rtx_def *" i.e.
> there's an implicit pointer. In the discussion about using C++
> classes for gimple statements:
> Richi said:
> > To followup myself here, it's because 'tree' is a typedef to a pointer
> > and thus 'const tree' is different from 'const tree_node *'.
> > Not sure why we re-introduced the 'mistake' of making 'tree' a pointer
> > when we introduced 'gimple'. If we were to make 'gimple' the class
> > type itself we can use gimple *, const gimple * and also const gimple &
> > (when a NULL pointer is not expected).
I guess it came from the time when gimple was a union in C, so you'd
have to write union gimple_seq * which is a bit weird.
> So in the following patches the pointerness is explicit: the patches
> refer to:
> rtx_insn *insn;
> rather than just:
> rtx_insn insn;
> and so one can write:
> const rtx_insn *insn
> and the "constness" applies to the insn, not to the pointer.
> But we could go either way here: the class could be "rtx_insn_def",
> with "rtx_insn" a typedef to an "rtx_insn_def *" etc with:
personally I prefer it the way it is.
> * Should as_a <rtx_insn *> accept a NULL pointer? It's possible to make
> the is_a_helper cope with NULL, but this adds an extra conditional.
I'd prefer it didn't accept NULL.
> I instead added an as_a_nullable<> cast, so that you can explicitly
> add a check against NULL before checking the code of the rtx node.
The name seems a little awkward (what exactly is nullable?) but I don't
have a better one.
> As for the performance of a regular build i.e. with as_a<>
> checks *enabled*; looking at the wallclock time taken for a bootstrap and
> regression test, for my s390 builds (with -j3) I saw:
I'm curious did you look at --enable-checking=rtl ? Did you manage to
remove some of those checks or is that future work?
> s390 control:
> "make" time: 68 mins
> "make check" time: 122 mins
> total time: 190 mins
> s390 experiment:
> "make" time: 70 mins
> "make check" time: 126 mins
> total time: 196 mins
> showing a 3% increase, presumably due to the as_a and as_a_nullable
> i.e. a release build shows no change in performance; a debug build shows
> a 3% increase in time taken to bootstrap and regression test. I believe
> the debug build could be sped up with further patches to eliminate the
> checked casts.
sounds reasonable especially if this takes over some of the role of rtl
It all looks reasonable from my skim of this series.