This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 000/236] Introduce rtx subclasses


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:
> http://dmalcolm.fedorapeople.org/presentations/cauldron-2014/rtl
> 
> They can also be seen at:
> https://dmalcolm.fedorapeople.org/gcc/patch-backups/rtx-classes/v20/
> 
> 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
>     thing:
> 
>       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:
>        https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01427.html
>     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
> checks.
> 
> 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
checking.

It all looks reasonable from my skim of this series.

Trev


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]