This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: RFC: MIPS paired single vector support


On Mon, 2004-08-16 at 13:32, Fu, Chao-Ying wrote:
> Here is our patch to support paired-single and MIPS-3D instructions.
> mips-ps-3d.diff, mips-ps-3d.md, and 14 tests are attached.

That is a nice collection of tests.  With this many tests, we probably
should start a gcc.dg/mips subdirectory for mips specific tests.  We
could perhaps even add testsuite support for it, so we don't have to put
"target mips*-*-*" in all of the files, but that is something we could
worry about later.

Some of the tests look more like demonstration code than compiler
tests.  Especially the one that codes matrix multiply 3 different ways.

The alnv.ps tests seem to miss the point of having this instruction, as
they just pass constants 0 or 4 to the builtin.  In a real use, you will
be passing in an address.  Also, alnv.ps is pretty useless without
lux/sux support, which I don't see in your patches.

There are no -ffast-math tests.  That might show up some problems.  This
failing is more obvious for the SB-1 support though, as the SB-1 has
recip.ps, and you can't generate that without using -ffast-math, so I
had to test the -ffast-math support with my patches.

The new mips-ps-3d.md file is a nice idea.  I should have thought of
doing that too.

I believe the TARGET_PAIRED_SINGLE checks in mips_arg_info are
redundant.  Since VECTOR_MODE_SUPPORTED_P already checks
TARGET_PAIRED_SINGLE, you will never have a vector mode if there is no
paired single support.

Likewise in override_options and mips_function_value.

You are lacking an ISA test for TARGET_PAIRED_SINGLE.  mips3/mips4 have
64-bit FP registers, but don't have the paired single instructions, so
we need to check for ISA_MIPS64.

The compare and builtin support is generally better than my patch, as
these parts are far from finished in my patch.

There are an awful lot of unspecs defined for these instructions.  You
really should be describing instructions with rtl operators whenever
possible.  Some of the stuff you have like UNSPEC_CVT_S_PL can be done
with vec_select which is better.  Actually, I see you have vec_select
commented out in the pattern, and it is commented out because of a bug. 
What bug?  I find this curious, as there is no optimization support for
vector RTL at the moment, so it seems unlikely that this could trigger a
bug.  Maybe an endian problem?

Your movv2sf pattern has support for handling zero that won't work,
because you haven't modified the constraints or predicates to accept a
vector 0 as a constant.

You don't have canonical -ffast-math nmadd/nmsub patterns.

All of the length "4" attributes are unnecessary.  4 is the default
length.  You only specify the length if it is different.

The equality_op references are a little curious.  It looks like this is
a post define_predicate patch, but equality_op got renamed to
equality_operator in the define_predicate patch.  Maybe a minor merging
or patching error?

You have a vec_init pattern, but no vec_set or vec_extract patterns.

I see you tried to handle endianness in the vec_init pattern.  Currently
the meaning of the vector RTL operators is undefined with respect to
endianness.  I didn't try to handle this, but then I had gotten around
to testing little-endian support yet.

alnv can be represented in RTL with a shift or a rotate.  Not clear if
an unspec is the best choice for it.

The cabs instructions could be something like
  (compare (abs (match_operand float register))
           (abs (match_operand float register)))
If we describe the RTL, then the compiler has a chance to do some
optimizations like substituting in constants and simplifying it.

For now, it is probably OK to have all of these unspecs, we can always
replace them with more descriptive RTL later.

It looks like you use a trick for eq:CCV2 and ne:CCV2.  You are
comparing against numbers 0 to 3, where each number means something
special.  This really should be documented somewhere.  This is unlikely
to work if we ever get real vector compare rtl support, as the machine
independent code is unlikely to do the same thing as your magic
numbers.  Even without vector support, there is the risk of some rtl
pass trying to invert a branch test, and then giving buggy code as it
doesn't know about your magic numbers.

Overall, there doesn't seem to be too much difference between our
patches.  We solved most of the problems the same way.  I think I have a
slightly better support for the basic operations, and you obviously have
much better support for compares and builtin functions, as that part of
my patch is unfinished.  Also, I have support for SB-1 extensions, but
that is pretty easy to drop on top of the basic paired single support.

Moving forward, it might be easiest to merge my improvements into your
patch, and then I can submit the SB-1 extensions as a second patch.  Or
maybe this patch can go in more or less as is, and I can submit bug fix
patches to improve it.  Along with a patch for the SB-1 extensions.

It would be nice if we had some verification that the copyright
assignments are OK now.  I will ask the FSF copyright clerk to verify
this.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]