regclass.c: scan_one_insn special casing

regclass.c: scan_one_insn special casing

[Eric Christopher]

There's this code in scan_one_insn:

 /* Improve handling of two-address insns such as
     (set X (ashift CONST Y)) where CONST must be made to
     match X. Change it into two insns: (set X CONST)
     (set X (ashift X Y)).  If we left this for reloading, it
     would probably get three insns because X and Y might go
     in the same place. This prevents X and Y from receiving
     the same hard reg.
                                                                                
     We can only do this if the modes of operands 0 and 1
     (which might not be the same) are tieable and we only need
     do this during our first pass.  */
                                                                                
  if (pass == 0 && optimize
      && recog_data.n_operands >= 3
      && recog_data.constraints[1][0] == '0'
      && recog_data.constraints[1][1] == 0
      && CONSTANT_P (recog_data.operand[1])
      && ! rtx_equal_p (recog_data.operand[0], recog_data.operand[1])
      && ! rtx_equal_p (recog_data.operand[0], recog_data.operand[2])
      && GET_CODE (recog_data.operand[0]) == REG
      && MODES_TIEABLE_P (GET_MODE (recog_data.operand[0]),
                          recog_data.operand_mode[1]))
    {

....


That appears to do something a port specific splitter would do. Now,
I've also looked and mips has a splitter that does something similar,
I'm not sure about other ports. I'm also really curious what port this
was added in for and perhaps the testcase...

I added an abort () right after the if statement above, and bootstrapped
and tested on x86-linux, mips-elf, sh-elf, arm-elf and couldn't trip it.

Any thoughts? I also have a patch to just remove it.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Michael Matz]

Hi,

On Sat, 27 Mar 2004, Eric Christopher wrote:

> There's this code in scan_one_insn:
> 
> 
> That appears to do something a port specific splitter would do.

I disagree.  It's in the generic code specifically that _no_ port specific
splitters have to do this.  If there are splitters just doing this they
should be removed.  And if they do something more it should be analyzed
why they are doing this and it should be considered to add this to the
generic code in order to enable the removing of the port specific hacks.

If anything the place of that code seems to be a bit strange, at least
from current perspective.  After these many years we might have a better
place.  OTOH it has to examine the insn constraints and the earliest point
GCC is doing this is in regclass.  Clearly it would be even better if
reload wouldn't do the stupid thing which this code is supposed to help.

> I've also looked and mips has a splitter that does something similar,
> I'm not sure about other ports. I'm also really curious what port this
> was added in for and perhaps the testcase...

Well, the comment is quite clear.  For all two address machines.  I would
be more interested to know why port specifics doing the same (or something
similar) were added.

> I added an abort () right after the if statement above, and bootstrapped
> and tested on x86-linux, mips-elf, sh-elf, arm-elf and couldn't trip it.

Probably because of the define_splits?


Ciao,
Michael.

Re: regclass.c: scan_one_insn special casing

[Andrew Pinski]

On Mar 29, 2004, at 00:59, Michael Matz wrote:

> Hi,
>
> On Sat, 27 Mar 2004, Eric Christopher wrote:
>
>> There's this code in scan_one_insn:
>>
>>
>> That appears to do something a port specific splitter would do.

I had looked into the history of this code and it was added before
define_splits were even in GCC.


> I disagree.  It's in the generic code specifically that _no_ port 
> specific
> splitters have to do this.  If there are splitters just doing this they
> should be removed.  And if they do something more it should be analyzed
> why they are doing this and it should be considered to add this to the
> generic code in order to enable the removing of the port specific 
> hacks.

The reason why it is generic code not specific for any port splitter is
because it was added before splitters were added so there was no way,
for port splitters to have been added.  It was in GCC 1.21 which
was before define_split was added (and before i386 was even added to
GCC, also).


> Well, the comment is quite clear.  For all two address machines.  I 
> would
> be more interested to know why port specifics doing the same (or 
> something
> similar) were added.

I would be interested too because this code has been there for a long 
time
before splitters were even thought of.  (The only thing I can think of 
is
they were added when the port was being designed as they did not know 
this
code existed).

Some times getting rid of code like this is the right thing to do as it
would make the compiler fast for target which will never invoke this
code at all.

Thanks,
Andrew Pinski

Re: regclass.c: scan_one_insn special casing

[David Edelsohn]

>>>>> Michael Matz writes:

Michael> If anything the place of that code seems to be a bit strange, at least
Michael> from current perspective.  After these many years we might have a better
Michael> place.  OTOH it has to examine the insn constraints and the earliest point
Michael> GCC is doing this is in regclass.  Clearly it would be even better if
Michael> reload wouldn't do the stupid thing which this code is supposed to help.

	This discussion thread appears to be mixing two issues: the code
appearing in a common part of the compiler and these types of special
cases scattered around the compiler.  The diffusion of this type of code
is a maintenance problem, but a single implementation should be a
benefit.

	If we could collect this type of functionality in an efficient
implementation in a central, well-defined location, GCC would be better.

David

Re: regclass.c: scan_one_insn special casing

[Eric Christopher]

On Mon, 2004-03-29 at 07:14, David Edelsohn wrote:
> >>>>> Michael Matz writes:
> 
> Michael> If anything the place of that code seems to be a bit strange, at least
> Michael> from current perspective.  After these many years we might have a better
> Michael> place.  OTOH it has to examine the insn constraints and the earliest point
> Michael> GCC is doing this is in regclass.  Clearly it would be even better if
> Michael> reload wouldn't do the stupid thing which this code is supposed to help.
> 
> 	This discussion thread appears to be mixing two issues: the code
> appearing in a common part of the compiler and these types of special
> cases scattered around the compiler.  The diffusion of this type of code
> is a maintenance problem, but a single implementation should be a
> benefit.

Right. I thought it seemed more of a waste in scan_one_insn than
anything else. Maybe an "optimize address" type of function would be
best. Especially since this optimization isn't dependent on "optimize"
at all.

Though, there's the "don't split _this_ insn, we do that somewhere else"
weird effect, but the comment is likely different than the reason we're
splitting it in the backend.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Joern Rennecke]

> That appears to do something a port specific splitter would do. Now,
> I've also looked and mips has a splitter that does something similar,
> I'm not sure about other ports. I'm also really curious what port this
> was added in for and perhaps the testcase...

What mips.md splitter are you talking about?

> I added an abort () right after the if statement above, and bootstrapped
> and tested on x86-linux, mips-elf, sh-elf, arm-elf and couldn't trip it.

I am not surprised that you couldn't trigger it on sh-elf.  The
arithmetic/logical instructions that allow constant operands do so only
for the second input operand.
The instruction that is tested for is not even valid for the port.
(The transformation is done by define_expands, or in the case of having
 just a define_insn with a predicate disallowing constants for operand 1,
 by the generic rtl generation code.)
Actually, I can't think of a port where it is valid.

As to the question where else we could have put this optimization if it
had merit, we could have put it into regmove.

Re: regclass.c: scan_one_insn special casing

[Eric Christopher]

On Tue, 2004-03-30 at 10:33, Joern Rennecke wrote:
> > That appears to do something a port specific splitter would do. Now,
> > I've also looked and mips has a splitter that does something similar,
> > I'm not sure about other ports. I'm also really curious what port this
> > was added in for and perhaps the testcase...
> 
> What mips.md splitter are you talking about?

I think I was mistaken, I looked again and couldn't find one that would
do the same split...

> Actually, I can't think of a port where it is valid.
> 

Me either now.

> As to the question where else we could have put this optimization if it
> had merit, we could have put it into regmove.

Anyhow, I'm in favor of removing it. Thanks for looking at it too.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Eric Christopher]

>     So that you aren't penalizing every call to scan_one_insn with this
>     code that is almost never called.
> 
> But that's a small cost compared to the maintenance issue of having
> that define_split in every port.

Which only runs if it is cost effective for the port, instead of your
code which checks every insn at least once. Generic algorithms in the
middle end and port specifics in the backend.

> 
> Now, you could certainly claim that there's some reason that this
> transformation can't occur (experimental isn't that interesting here)
> and that the code is therefore obsolete. But I haven't seen that claim.

Of course, you can pull it around and make someone justify that the code
should exist - especially given the compile time penalty. A testcase
that proves it can be called and is in the testsuite as an example.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Richard Kenner]

    > For those very few cases that are machine-specific.

    There are tons of things that go through define_split now. You should
    look at a few ports as examples:

I know.  It has been used a lot, some would say abused.  But that's no
reason to require each port to add yet more of them.

    I know, you put it in there in 92. I won't ask why - I don't even want
    to try to think of things I did 11 years ago :)

Actually, I *thought* I wrote that, but wasn't sure.

    So that you aren't penalizing every call to scan_one_insn with this
    code that is almost never called.

But that's a small cost compared to the maintenance issue of having
that define_split in every port.

Now, you could certainly claim that there's some reason that this
transformation can't occur (experimental isn't that interesting here)
and that the code is therefore obsolete. But I haven't seen that claim.

Re: regclass.c: scan_one_insn special casing

[Eric Christopher]

> > But I feel that this case should be handled right where ti is and that's
> > the best place to put it.
> > 
> 
> I know, you put it in there in 92. I won't ask why - I don't even want
> to try to think of things I did 11 years ago :)

Apparently it predates that too.

1.1          (kenner   05-Nov-91):            /*

And that's from old-gcc.

I have no idea when this code was added if it predates that.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Eric Christopher]

> For those very few cases that are machine-specific.
> 

There are tons of things that go through define_split now. You should
look at a few ports as examples:

[echristo@dzur sh]$ grep define_insn sh.md | wc -l
    413
[echristo@dzur sh]$ grep define_split sh.md | wc -l
     56

[echristo@dzur mips]$ grep define_split mips.md | wc -l
     60
[echristo@dzur mips]$ grep define_insn mips.md | wc -l
    366

>     There's just something wrong with special casing all over the compiler
>     just for a small case that could/should be handled somewhere else.
> 
> But I feel that this case should be handled right where ti is and that's
> the best place to put it.
> 

I know, you put it in there in 92. I won't ask why - I don't even want
to try to think of things I did 11 years ago :)

> How is it better to put this in a define_split in each port?

So that you aren't penalizing every call to scan_one_insn with this code
that is almost never called. (I'll say almost never even though I've not
tripped it with 4 ports and all of check-gcc). It also hurts
maintainability - scatter 100 or so of these cases through the compiler
and it looks like spaghetti.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Richard Kenner]

    > Sure, but it's better to do something in *machine-independent* code if
    > you can.

    Then why bother having define_split?

For those very few cases that are machine-specific.

    There's just something wrong with special casing all over the compiler
    just for a small case that could/should be handled somewhere else.

But I feel that this case should be handled right where ti is and that's
the best place to put it.

How is it better to put this in a define_split in each port?

Re: regclass.c: scan_one_insn special casing

[Eric Christopher]

On Sat, 2004-03-27 at 13:44, Richard Kenner wrote:
>     That appears to do something a port specific splitter would do. 
> 
> Sure, but it's better to do something in *machine-independent* code if
> you can.
> 

Then why bother having define_split?

>     Any thoughts? I also have a patch to just remove it.
> 
> Why?
> 
> "If it ain't broke, don't fix it."

There's just something wrong with special casing all over the compiler
just for a small case that could/should be handled somewhere else. There
seem to be a lot of this "special casing" for things that should be done
somewhere else and help maintainability. And hell, if we don't have to
do those 7 if checks we would improve the speed of the compiler - in a
small way at least. If we remove 10 such cases we've probably started in
on a large such improvement.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: regclass.c: scan_one_insn special casing

[Richard Kenner]

    That appears to do something a port specific splitter would do. 

Sure, but it's better to do something in *machine-independent* code if
you can.

    Any thoughts? I also have a patch to just remove it.

Why?

"If it ain't broke, don't fix it."