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
