Re: RFA: MIPS paired single support

[James E Wilson <wilson at specifixinc dot com>]

On Tue, 2004-08-24 at 05:25, Richard Sandiford wrote:
> Sorry, but I don't like this at all.  One of your main reasons for using
> things like vec_merge rather than UNSPECs was that it was possible to
> describe the operation accurately with standard rtl codes.  If you're
> saying that it _isn't_ possible to describe some of the patterns accurately
> with standard rtl codes, please use UNSPECs for those patterns until the
> situation changes.

I think you are headed in the wrong direction here.  There are a lot of
problems in the middle end with the vector support.  If I have to find
and fix them all before we can add proper vector patterns to the mips.md
file, then this is a hopeless task.  I think the right thing to do with
the mips.md file is be consistent with all of the other ports that
define vector support, until such time as the middle end issues are
worked out, and that is exactly what I have done.

I would rather make forward progress on the vector support, rather than
hide bugs by using UNSPECs.

> I still prefer 'YG'.

OK.  This is done now.

>And "yet another" seem a bit uncalled-for.  If we add a mode macro 
>SF/DF/V2SF, we'll need a mode attribute for the fmt (s, d, ps) and,
>yes, an attribute for the (set_attr "mode" ...).  I still claim that
>this is neater than copying and pasting each arithmetic pattern and
>making the _exact same_ changes by hand.

You misunderstood my point here.  There was no intent to disparage
anything.  The point I was trying to make was that if we add V2SF to the
DFA schedulers, then we can get away with at least one less
define_mode_macro.  There is an obvious trade off here, and I felt
obligated to point it out, so you could decide which approach you
preferred.

I never said that there was anything wrong with using the
define_mode_macro stuff.  Only that there was a choice that needed to be
made here.

> Not in recent versions of gcc.  Unnamed define_insns get named after the
> file and line number of the pattern definition (see read-rtl.c).  "+n" names
> are only used for things like define_splits, which have no named at all at
> the rtl level.

Right.  I forgot about that.

> Yes ;).  Please only use "fadd" for operations that actually do addition.
> Please use "fmove" for instructions that move things about and "fcvt" for
> instructions that perform some kind of numerical conversion.

OK.  This is fixed.

> Thanks for the explanation.  I still disagree with the conclusion though.
> There doesn't seem to be any way to test whether the pattern actually works,
> and there's no guarantee that any future extensions will want to use the
> pattern in its current form.  I object to adding dead code.

And I still have to disagree with you.  I am trying to make forward
progress on the vector support in both the middle and back ends, and I
think you are putting obstacles in my way.  You are also insisting that
the mips backend be written to a different standard than is being used
for the other backends.

It is trivial to test the vec_extract pattern.  All you have to do is
perform a V2SF divide for a non-SB-1 target.  No source code changes
required.  I have done this, and it works.

Furthermore, the existence of this pattern is critical to getting good
performance for this example.  This is a real world example that some
end users will need.  I can not compromise on this issue.

> Same comment applies here.

This is for vec_set.  Again, I think you are obstructing forward
progress here.  Adding a vec_set pattern is the right thing to do, even
if currently there are weaknesses in the middle end vector support.

Also, you are assuming that if I can't find an obvious way to trigger,
than there are none.  It may be that there are some I haven't found yet.

It is possible to test this patch.  Disable the vec_init pattern, and
then perform a divide for a non-SB-1 target.  I have done this, and the
patterns works.

I will concede that this is not a very convincing argument, but I still
don't like the idea of intentionally crippling the mips vector support.

> Disagree ;)  It's easy to add extra "type" attributes.  The sb1.md
> change can be separate if you like...

It is easy to add extra type attributes, but the required DFA scheduler
changes makes the change tedious, and results in large patches.  My
change to add the frdiv type attribute was almost 17K.  I have to add 4
new type attributes to handle the recip/sqrt step instructions
correctly.  The float vector patch is already too large (180K+).  I
think it is a mistake to add another large patch into it.  

> > I could pull out these 6 sections of code into a small subroutine. 
> > Would that be OK.
> Sure would, thanks.

OK.  This is done.

>Besides, gcc can now inline small static functions automatically, so I
>refuse to believe that the performance is an issue here.

Sure, but there may be overhead for other reasons, since I have to
generalize the code to make this work.  In any case, I am just trying to
present all of the options and arguments to you, so you can make a
decision.  Different people have different opinions on different
subjects, and since you are choosing to micro-manage the patch review, I
have to bring up every issue no matter how trivial it may seem.

> You didn't seem to answer my main suggestion though, which is that the
> MIPS_BUILTIN_s shouldn't be needed.  It's just an ordinary 0-based
> enumeration.  For example, instead of:

I didn't understand exactly what you were asking for, and there are so
many other requests you are making.

Note that the way this patch is written is similar to how other back
ends work, i.e. it is following existing practice.  I am a little
puzzled why you think the MIPS port should work differently than all
other ports.  However, I think this is moot, as I probably won't have
time to try changing this.

Note that some builtin functions do not expand a single unique patterns
in the md file, so it may not be possible to completely eliminate the
MIPS_BUILTIN_* enum.

> But I think it's reasonable to expect at least _some_ indication of what
> (in gcc terms) "support for paired single instructions" means.

OK, added to the list.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com