Use predicates for RTL objects

Use predicates for RTL objects

[Arvind Sankar]

Hi,
I have posted a patch series [1] for converting some of the RTL code to
use predicate macros, as described in the suggestions for beginner GCC
projects [2]. Segher was kind enough to give some comments on the
initial posting [3].

The code has been bootstrapped natively on x86_64, and I have built
cross-compilers for all targets except tilegx which gave build errors
even on trunk. The compiler object files are identical with trunk except
for *-checksum.o and string tables in build/gen*.o which change from
GET_CODE (..) == .. etc to the predicate macro.

Not all possible changes have been made yet, I figured I'd send this out
to check first.

I am hoping one of the maintainers will be able to take some time to
review the patches -- a few are quite large but are mechanical.

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)

More complex predicates could be left as macros or additional methods
could be defined like rtx_def::is_a_nondebug_insn etc. I think this
should mostly be an improvement, although the comparisons around INSN
may become slightly more confusing: currently, INSN_P (x) is different
from is_a <rtx_insn *> (x), and using something like x->is_a_insn () for
the former would probably increase confusion.

Thanks.

[1] https://gcc.gnu.org/ml/gcc-patches/2019-08/msg00327.html
[2] https://gcc.gnu.org/projects/beginner.html
[3] https://gcc.gnu.org/ml/gcc-patches/2019-08/msg00171.html

Re: Use predicates for RTL objects

[Segher Boessenkool]

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.

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!

(And "REG_P" and similar are much shorter code to type).


Segher

Re: Use predicates for RTL objects

[Arvind Sankar]

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.

> 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
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. 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.

> 
> (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.

> 
> Segher

Re: Use predicates for RTL objects

[Segher Boessenkool]

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.

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.


Segher

Re: Use predicates for RTL objects

[Arvind Sankar]

On Wed, Aug 07, 2019 at 01:05:51PM -0500, Segher Boessenkool wrote:
> 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.

True, but the code is semantically a type identifier. Using the common
is_a idiom IMO makes it more readable for that reason, but that is a
matter of personal opinion, hence my request for comments.

> 
> > > 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!
> > 
...
> > 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.

My reply is pointing out that it is just as (non-)obvious with or
without that inline function, if you want to use any of the helper
macros. It wouldn't be a reasonable argument to say that INSN_P (x) is
obviously efficient while x->is_a (PLUS) is hiding some potentially
nasty things inside. There's around 200 uses of GET_CODE being compared
to PLUS in the non-target-specific code, so it's not that rare. It would
be nice to replace it with something better, but PLUS_P (x) just reads
badly to me.

> 
> > 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.

I'm not saying that we don't care about performance. My point is that
if you know that what you're checking is just whether this RTX is a
PLUS-coded RTX or not, you should not care exactly which machine
instructions that check will generate, because you already know that the
test should be trivial, and any sensible implementation will be
efficient enough, especially after a compiler is done with it. i.e.
being sure of getting efficient machine code is not a reason for
avoiding macros/inline functions.

Re: Use predicates for RTL objects

[Richard Sandiford]

Segher Boessenkool <segher@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

Re: Use predicates for RTL objects

[Jeff Law]

On 8/7/19 12:05 PM, Segher Boessenkool wrote:
> 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.
True, but they could be.  When David was working in this space a few
years ago I concluded that the main value in sub-classing the various
RTL operators just wansn't worth the effort.  Instead we focused on
starting to tear apart things like the toplevel objects into rtx_insn
and the like.  THere's little value in treating those as simple RTXs.
INSN_LIST and the like were also ripe for this treatment.

The biggest value in making a real class for the operators things would
be to move the runtime RTL checking into a compile-time check.  But I
couldn't really green light something like that without first completing
the rtx_insn changes.

> 
> 
> 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.
Yup.  And continue pushing the rtx_insn bits deeper, tackling INSN_LIST,
etc.

jeff

Re: Use predicates for RTL objects

[Michael Matz]

Hi,

On Wed, 7 Aug 2019, Arvind Sankar wrote:

> 	=> x->is_a (REG)

Oh god, please no.  Currently at least the RTL parts of GCC still have 
mostly a consistent and obvious style, which is a good thing.  I have no 
idea why anyone would think the above is easier to read than REG_P (x).


Ciao,
Michael.
P.S: Consider this: the current style served us quite well for the last 35 
or so years, so before suggesting style changes, shouldn't you first work 
on the sources for some time?

Re: Use predicates for RTL objects

[Segher Boessenkool]

On Wed, Aug 07, 2019 at 02:58:12PM -0400, Arvind Sankar wrote:
> > > > code does is cheap.  With "x->is_a (PLUS)", who knows what is happening
> > That's not my point -- my point was that it is *obvious* the way things
> > are now, which is nice.
> 
> My reply is pointing out that it is just as (non-)obvious with or
> without that inline function, if you want to use any of the helper
> macros.

But that is not what you suggested, or at least not how I read it.
  x->is_a (PLUS)
is not obviously cheap or simple at all, while
  GET_CODE (x) == PLUS
obviously *is*.

The former also isn't readable.

Indirection is *the* evil in programming.

All the common stuff that can be easily hidden behind macros, sure,
but we're not talking only about that.  And if macros have non-trivial
implementations, they shouldn't be macros (but inlines), or maybe
shouldn't even exist at all.

> > > 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.
> 
> I'm not saying that we don't care about performance.

That is now what I said, either.  Performance is only one aspect of
simplicity.


Segher

Re: Use predicates for RTL objects

[Segher Boessenkool]

On Thu, Aug 08, 2019 at 10:10:38AM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Wed, Aug 07, 2019 at 01:39:53PM -0400, Arvind Sankar wrote:
> >> 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...

_In principle_, sure, I have nothing against C++ conversions if it has
advantages that outweigh their disadvantages.

> > 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).

It is *the* core thing that needs to be changed if you want this to be
any other than pointless uglification.

Those two RTX fields are the core of how it works now.  If you want to
use C++ classes, instead, you need to replace them with some suitable
C++ thing.  And we need to see that design before we can say if it looks
acceptable at all.

> 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).

I would prefer to see an overall design before making partial changes.

> 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.

Many people enable it all the time.  It isn't so expensive compared to
other things.  --enable-checking=yes,rtl,tree saves more time than it
costs, ime, and "rtl" is not the most expensive in that.

> - More efficient layouts, rather than forcing every piece of information
>   outside the header to be in a pointer-sized field.

... and then the GC has to be tuned again.  Oh joy.  Well, it probably
is much detuned already, but :-)

It would be nice to see real numbers how much memory this would save, and
what that means for compiler runtime.


Segher

Re: Use predicates for RTL objects

[Segher Boessenkool]

On Thu, Aug 08, 2019 at 08:31:28AM -0600, Jeff Law wrote:
> On 8/7/19 12:05 PM, Segher Boessenkool wrote:
> > It is not a class in the C++ sense.  Confusing this is not useful for
> > anyone.
> True, but they could be.

But then it would say  is_a (rtx_plus)  or similar.  We don't have
classes called "PLUS" :-)

> When David was working in this space a few
> years ago I concluded that the main value in sub-classing the various
> RTL operators just wansn't worth the effort.  Instead we focused on
> starting to tear apart things like the toplevel objects into rtx_insn
> and the like.  THere's little value in treating those as simple RTXs.
> INSN_LIST and the like were also ripe for this treatment.

Yup.  And there still are various casts around, so even this conversion
still isn't complete (but it is not bad at all :-) )

> The biggest value in making a real class for the operators things would
> be to move the runtime RTL checking into a compile-time check.

That ignores 90% of the errors RTL checking catches: memory corruption.

> > 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.
> Yup.  And continue pushing the rtx_insn bits deeper, tackling INSN_LIST,
> etc.

I'm all for it.

What I do *not* want to see is a partial conversion to C++ classes, doing
the easy bits, and leaving a gigantic mess for everything else.


Segher

Re: Use predicates for RTL objects

[Arvind Sankar]

On Thu, Aug 08, 2019 at 08:31:28AM -0600, Jeff Law wrote:
> True, but they could be.  When David was working in this space a few
> years ago I concluded that the main value in sub-classing the various
> RTL operators just wansn't worth the effort.  Instead we focused on
> starting to tear apart things like the toplevel objects into rtx_insn
> and the like.  THere's little value in treating those as simple RTXs.
> INSN_LIST and the like were also ripe for this treatment.
> 
> The biggest value in making a real class for the operators things would
> be to move the runtime RTL checking into a compile-time check.  But I
> couldn't really green light something like that without first completing
> the rtx_insn changes.

Are there any notes or old discussion threads on what remains? I would
be interested in taking a look if no-one else is.

Thanks

> 
> > 
> > 
> > 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.
> Yup.  And continue pushing the rtx_insn bits deeper, tackling INSN_LIST,
> etc.
> 
> jeff

Re: Use predicates for RTL objects

[Arvind Sankar]

On Thu, Aug 08, 2019 at 03:04:53PM +0000, Michael Matz wrote:
> Hi,
> 
> On Wed, 7 Aug 2019, Arvind Sankar wrote:
> 
> > 	=> x->is_a (REG)
> 
> Oh god, please no.  Currently at least the RTL parts of GCC still have 
> mostly a consistent and obvious style, which is a good thing.  I have no 
> idea why anyone would think the above is easier to read than REG_P (x).
> 
> 
> Ciao,
> Michael.
> P.S: Consider this: the current style served us quite well for the last 35 
> or so years, so before suggesting style changes, shouldn't you first work 
> on the sources for some time?

Well, the main point of the email was to ask for review of a patchset
that attempts to make progress on a TODO that has been outstanding for
at least 15 of those 35 years.

Re: Use predicates for RTL objects

[Arvind Sankar]

On Thu, Aug 08, 2019 at 10:04:14AM -0500, Segher Boessenkool wrote:
> On Wed, Aug 07, 2019 at 02:58:12PM -0400, Arvind Sankar wrote:
> > > > > code does is cheap.  With "x->is_a (PLUS)", who knows what is happening
> > > That's not my point -- my point was that it is *obvious* the way things
> > > are now, which is nice.
> > 
> > My reply is pointing out that it is just as (non-)obvious with or
> > without that inline function, if you want to use any of the helper
> > macros.
> 
> But that is not what you suggested, or at least not how I read it.
>   x->is_a (PLUS)
> is not obviously cheap or simple at all, while
>   GET_CODE (x) == PLUS
> obviously *is*.
> 
> The former also isn't readable.

That's a matter of what style you prefer and it seems like no-one else
shares my preference, and I accept that we're not going to make such a
change.

But there is really nothing more or less obvious about it. It's easy to
go look at the code, as you probably once did when checking what
GET_CODE or REG_P actually did, and is_a methods are expected to be
lightweight. Regarding hiding things, consider that we just added
LABEL_REF_P, which is for a comparison that happens less than half as
often as PLUS in the codebase (and I think it was actually only
used in one place). It was done presumably because the author/reviewers
felt that LABEL_REF_P (x) is more readable than GET_CODE (x) == LABEL_REF, 
even if the latter might be ever so slightly more transparent as to what
its doing than the former.

> 
> Indirection is *the* evil in programming.

Some would say that "All problems in computer science can be solved by
another level of indirection" ;)

Re: Use predicates for RTL objects

[Jakub Jelinek]

On Thu, Aug 08, 2019 at 01:12:33PM -0400, Arvind Sankar wrote:
> On Thu, Aug 08, 2019 at 03:04:53PM +0000, Michael Matz wrote:
> > Hi,
> > 
> > On Wed, 7 Aug 2019, Arvind Sankar wrote:
> > 
> > > 	=> x->is_a (REG)
> > 
> > Oh god, please no.  Currently at least the RTL parts of GCC still have 
> > mostly a consistent and obvious style, which is a good thing.  I have no 
> > idea why anyone would think the above is easier to read than REG_P (x).
> > 
> > 
> > Ciao,
> > Michael.
> > P.S: Consider this: the current style served us quite well for the last 35 
> > or so years, so before suggesting style changes, shouldn't you first work 
> > on the sources for some time?
> 
> Well, the main point of the email was to ask for review of a patchset
> that attempts to make progress on a TODO that has been outstanding for
> at least 15 of those 35 years.

Not everything that is in some TODO list somewhere is something generally
agreed upon.

	Jakub

Re: Use predicates for RTL objects

[Segher Boessenkool]

On Thu, Aug 08, 2019 at 01:42:40PM -0400, Arvind Sankar wrote:
> On Thu, Aug 08, 2019 at 10:04:14AM -0500, Segher Boessenkool wrote:
> But there is really nothing more or less obvious about it.

That depends on what you are used to seeing, a lot.

> It's easy to
> go look at the code, as you probably once did when checking what
> GET_CODE or REG_P actually did,

It's in the documentation as well, and there are examples every tenth
line of code to look at.

> and is_a methods are expected to be
> lightweight. Regarding hiding things, consider that we just added
> LABEL_REF_P, which is for a comparison that happens less than half as
> often as PLUS in the codebase (and I think it was actually only
> used in one place). It was done presumably because the author/reviewers
> felt that LABEL_REF_P (x) is more readable than GET_CODE (x) == LABEL_REF, 
> even if the latter might be ever so slightly more transparent as to what
> its doing than the former.

LABEL_REF_P works out nicely because it is referring to something that
is data, is not an operator.  "Leaves" in an RTL expression, if you want
to look at it that way.

Predicates for other RTX codes aren't always as obvious, see CONST_P as
example.  PLUS_P would be a bit borderline.

Part of the reason why REG_P and MEM_P and the like are nice, is that
these predicates are often used in bigger conditions, maybe together
with some XEXP and whatnot.  Is that the case for PLUS_P?

> > Indirection is *the* evil in programming.
> 
> Some would say that "All problems in computer science can be solved by
> another level of indirection" ;)

Yes, and that is the problem.  :-P

Indirection doesn't solve problems, it just hides them.  Diluting your
code does not make it simpler, quite the opposite; it just makes it hard
to spot the problem parts.


Segher

Re: Use predicates for RTL objects

[Arvind Sankar]

On Thu, Aug 08, 2019 at 08:14:01PM +0200, Jakub Jelinek wrote:
> On Thu, Aug 08, 2019 at 01:12:33PM -0400, Arvind Sankar wrote:
> > On Thu, Aug 08, 2019 at 03:04:53PM +0000, Michael Matz wrote:
> > > Hi,
> > > 
> > > On Wed, 7 Aug 2019, Arvind Sankar wrote:
> > > 
> > > > 	=> x->is_a (REG)
> > > 
> > > Oh god, please no.  Currently at least the RTL parts of GCC still have 
> > > mostly a consistent and obvious style, which is a good thing.  I have no 
> > > idea why anyone would think the above is easier to read than REG_P (x).
> > > 
> > > 
> > > Ciao,
> > > Michael.
> > > P.S: Consider this: the current style served us quite well for the last 35 
> > > or so years, so before suggesting style changes, shouldn't you first work 
> > > on the sources for some time?
> > 
> > Well, the main point of the email was to ask for review of a patchset
> > that attempts to make progress on a TODO that has been outstanding for
> > at least 15 of those 35 years.
> 
> Not everything that is in some TODO list somewhere is something generally
> agreed upon.
> 
> 	Jakub

Surely there's general agreement on using REG_P etc? I don't see anyone
objecting to it, and that's all the patchset does: to avoid any
confusion the second half of the email asking about opinions on is_a is
entirely independent from the first half describing the existing patchset.

Re: Use predicates for RTL objects

[Jakub Jelinek]

On Thu, Aug 08, 2019 at 02:35:27PM -0400, Arvind Sankar wrote:
> Surely there's general agreement on using REG_P etc? I don't see anyone

No objections from me for using REG_P and other *_P macros more.

> objecting to it, and that's all the patchset does: to avoid any
> confusion the second half of the email asking about opinions on is_a is
> entirely independent from the first half describing the existing patchset.

My comment was mainly targetted at the ->is_a stuff, but also a general
comment that having something written in some wiki doesn't mean there is
agreement on it.

	Jakub

Re: Use predicates for RTL objects

[Arvind Sankar]

On Thu, Aug 08, 2019 at 01:26:42PM -0500, Segher Boessenkool wrote:
> LABEL_REF_P works out nicely because it is referring to something that
> is data, is not an operator.  "Leaves" in an RTL expression, if you want
> to look at it that way.
> 
> Predicates for other RTX codes aren't always as obvious, see CONST_P as
> example.  PLUS_P would be a bit borderline.
> 
> Part of the reason why REG_P and MEM_P and the like are nice, is that
> these predicates are often used in bigger conditions, maybe together
> with some XEXP and whatnot.  Is that the case for PLUS_P?
> 

Yes, it's used quite often in more complex conditions checking the
operands (eg to see whether they're constants), or applied to XEXP's
itself.

But I'm in agreement that PLUS_P just seems odd somehow. The leaf/data
vs operator distinction makes sense, maybe RTXOP_PLUS_P, but then you'd
want that to check if it was being called on an operator, so I don't
know if you'd do it unless/until we eventually have an rtx_op class and
have done the other bits of converting to C++.

Re: Use predicates for RTL objects

[Jeff Law]

On 8/8/19 11:06 AM, Arvind Sankar wrote:
> On Thu, Aug 08, 2019 at 08:31:28AM -0600, Jeff Law wrote:
>> True, but they could be.  When David was working in this space a few
>> years ago I concluded that the main value in sub-classing the various
>> RTL operators just wansn't worth the effort.  Instead we focused on
>> starting to tear apart things like the toplevel objects into rtx_insn
>> and the like.  THere's little value in treating those as simple RTXs.
>> INSN_LIST and the like were also ripe for this treatment.
>>
>> The biggest value in making a real class for the operators things would
>> be to move the runtime RTL checking into a compile-time check.  But I
>> couldn't really green light something like that without first completing
>> the rtx_insn changes.
> 
> Are there any notes or old discussion threads on what remains? I would
> be interested in taking a look if no-one else is.
I don't recall if that discussion was internal or external.  If the
latter, then it'd be in the gcc-patches archives.

Jeff

Re: Use predicates for RTL objects

[Jeff Law]

On 8/8/19 11:42 AM, Arvind Sankar wrote:
> On Thu, Aug 08, 2019 at 10:04:14AM -0500, Segher Boessenkool wrote:
>> On Wed, Aug 07, 2019 at 02:58:12PM -0400, Arvind Sankar wrote:
>>>>>> code does is cheap.  With "x->is_a (PLUS)", who knows what is happening
>>>> That's not my point -- my point was that it is *obvious* the way things
>>>> are now, which is nice.
>>>
>>> My reply is pointing out that it is just as (non-)obvious with or
>>> without that inline function, if you want to use any of the helper
>>> macros.
>>
>> But that is not what you suggested, or at least not how I read it.
>>   x->is_a (PLUS)
>> is not obviously cheap or simple at all, while
>>   GET_CODE (x) == PLUS
>> obviously *is*.
>>
>> The former also isn't readable.
> 
> That's a matter of what style you prefer and it seems like no-one else
> shares my preference, and I accept that we're not going to make such a
> change.
> 
> But there is really nothing more or less obvious about it. It's easy to
> go look at the code, as you probably once did when checking what
> GET_CODE or REG_P actually did, and is_a methods are expected to be
> lightweight. Regarding hiding things, consider that we just added
> LABEL_REF_P, which is for a comparison that happens less than half as
> often as PLUS in the codebase (and I think it was actually only
> used in one place). It was done presumably because the author/reviewers
> felt that LABEL_REF_P (x) is more readable than GET_CODE (x) == LABEL_REF, 
> even if the latter might be ever so slightly more transparent as to what
> its doing than the former.
I think what Segher is referring to is that GCC developers should know
that GET_CODE is just x->code, ie, a single dereference, similarly for
other RTL/TREE accessors.  A function call (or something that looks like
a function call) has traditionally been reserved for things that are
less performance sensitive and could be doing more work within GCC.

But those are merely conventions and ones that we've had in this project
for 30+ years and are to some degree driven by the C language as it
stood in the 80s.   We're not as strict about those conventions as we've
been in the past, but I don't think we're ready to throw them totally
out the window.

There's nothing *inherently* cheaper about either form.  Both could be
used in either way.

Jeff

Re: Use predicates for RTL objects

[Jeff Law]

On 8/8/19 12:46 PM, Jakub Jelinek wrote:
> On Thu, Aug 08, 2019 at 02:35:27PM -0400, Arvind Sankar wrote:
>> Surely there's general agreement on using REG_P etc? I don't see anyone
> 
> No objections from me for using REG_P and other *_P macros more.
Right.  These are convenience macros and I think they're generally a
good thing.  Other good examples would be things like POINTER_TYPE_P
which accepts POINTER_TYPE or REFERENCE type -- it's way to easy to do
something like TREE_TYPE (x) == POINTER_TYPE and thus miss handling
reference types.


> 
>> objecting to it, and that's all the patchset does: to avoid any
>> confusion the second half of the email asking about opinions on is_a is
>> entirely independent from the first half describing the existing patchset.
> 
> My comment was mainly targetted at the ->is_a stuff, but also a general
> comment that having something written in some wiki doesn't mean there is
> agreement on it.
Agreed on both points.

jeff

Re: Use predicates for RTL objects

[Richard Biener]

On Wed, Aug 7, 2019 at 7:34 PM Segher Boessenkool
<segher@kernel.crashing.org> 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.
>
> 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!
>
> (And "REG_P" and similar are much shorter code to type).

Note also that in other places in GCC we use

  is_a <REG> (x)

instead, see is-a.h.  I don't welcome your member-function style style :/

Richard.

>
> Segher

Re: Use predicates for RTL objects

[Ben Elliston]

> Do people still want (GET_CODE (rtx) == XXX) cleaned up to XXX_P(RTX).

I would think so -- did you get that from the beginner's project list?

> I would probably do all the plus/minus stuff first. Submit that as a patch.

Sure.  Please read the patch submission guidelines before starting.

> ps. Do I just send the diff file to this mailing list?

No, gcc-patches.

Cheers, Ben

Use predicates for RTL objects

[nathan bullock]

Do people still want (GET_CODE (rtx) == XXX) cleaned up to XXX_P(RTX).

If so I notice that there are quite a few of the following:

200 of
GET_CODE(rtx) == PLUS  ---> PLUS_P(rtx)

50 of
GET_CODE(rtx) == MINUS ----> MINUS_P(rtx)

550 of
GET_CODE(rtx) == CONST_INT ---> CONST_INT_P(rtx)

70 of
GET_CODE(rtx) == CONST_DOUBLE ----> CONST_DOUBLE_P(rtx)

150 of
GET_CODE(rtx) == PARALLEL ----> CONST_PARALLEL_P(rtx)

I would probably do all the plus/minus stuff first. Submit that as a patch.

Nathan Bullock

ps. Do I just send the diff file to this mailing list?