Segher Boessenkool <seg...@kernel.crashing.org> writes:
> On Wed, Aug 07, 2019 at 01:39:53PM -0400, Arvind Sankar wrote:
>> On Wed, Aug 07, 2019 at 12:33:53PM -0500, Segher Boessenkool wrote:
>> > On Wed, Aug 07, 2019 at 12:15:29PM -0400, Arvind Sankar wrote:
>> > > I would also like to get some comments on the following idea to make the
>> > > code checks more readable: I am thinking of adding
>> > > bool rtx_def::is_a (enum rtx_code) const
>> > > This would allow us to make all the rtx_code comparisons more readable
>> > > without having to define individual macros for each.
>> > > i.e.,
>> > > REG_P (x) => x->is_a (REG)
>> > > GET_CODE (x) == PLUS => x->is_a (PLUS)
>> > > GET_CODE (PATTERN (x)) == SEQUENCE => PATTERN (x)->is_a (SEQUENCE)
>> >
>> > That makes things much worse. Not only is it less readable (IMO), but
>> > the "is_a" idiom is used to check if something is of a certain class,
>> > which is not the case here.
>>
>> Well, the rtx_code *is* kind of a class. It determines what fields of
>> the rtx are valid and what they contain etc.
>
> It is not a class in the C++ sense. Confusing this is not useful for
> anyone.
>
>> > In "GET_CODE (x) == PLUS" it is clear that what the resulting machine
>> > code does is cheap. With "x->is_a (PLUS)", who knows what is happening
>> > below the covers!
>>
>> We already have, for eg, is_a <rtx_sequence *> (x), and there are
>
> Whis *is* a class. And not all of us are happy with that, but since we
> don't often have to see it at all, it's not so bad.
Speaking as someone who is happy about that[*]...
[*] ...at least in principle. It isn't really a proper class yet
because we don't construct rtx sequences as rtx_sequence objects,
we just access them that way. I was a bit surprised that this
was defined behaviour...
> Having rtx_insn a separate type from rtx is actually useful, btw.
>
>> predicate macros whose implementation is more complex than checking the
>> code field. You basically have to trust that it's sensibly implemented,
>> i.e. that it is as efficiently implemented as it can be.
>
> That's not my point -- my point was that it is *obvious* the way things
> are now, which is nice.
>
>> I don't think
>> people writing RTL transformations should be overly worried about what
>> machine code their predicates are generating, especially when
>> they're calling the defined API for it.
>
> The whole *design* of RTL is based around us caring a whole lot.
>
>> > (And "REG_P" and similar are much shorter code to type).
>>
>> That is true for the ones that exist, but there are lots more that don't
>> and it doesn't really make sense to add individual macros for all of
>> them.
>
> Yes. So use GET_CODE for those? REG_P is super frequent, it is really
> handy to have a macro for it.
>
>
> If you really want to convert RTL to C++, you should start with getting
> rid of rtx_format and rtx_class, and make REG_P etc. work just as they
> have always done.
I don't think getting rid of rtx_format and rtx_class should
necessarily be the first step. The base class can still provide
the traditional accessors (with runtime checking when enabled).
Another option would be to start adding derived classes for certain
types of rtx, and actually constructing those types of rtx with the
appropriate type. It'd probably make sense to start with special-
purposes rtxes like ADDRESS, DEBUG_IMPLICIT_PTR or SYMBOL_REF (which
is already somewhat special).
IMO the advantages of using a proper class hierarchy would be:
- More static type checking. Runtime RTL checking is still seen as too
expensive to enable by default even in development builds, so RTL goes
unchecked most of the time.
- More efficient layouts, rather than forcing every piece of information
outside the header to be in a pointer-sized field.
Thanks,
Richard
