Re: [PATCH, ARM] iWMMXT maintenance

[Ramana Radhakrishnan <ramana dot radhakrishnan at linaro dot org>]

Joseph S. Myers wrote:
> On Mon, 20 Jun 2011, Xinyu Qi wrote:
>
>> *gcc/config/arm/elf.h: Add option -mwmmxt.
>> *gcc/config/arm/arm.opt: Same.
>
> Why?  And where are the documentation updates?  How does this relate to
> the existing iWMMXt options?
>
> I thought the plan (Ramana?) was to move to having a -msimd= option to
> select SIMD units, reflecting that "neon" is not an FPU and "iwmmxt" is
> not an architecture (or a CPU).
>  So you'd have -march=armv5te
> -msimd=iwmmxt as replacement for the present -march=iwmmxt - or
> -mcpu=<whatever> implying that combination of options.  Given that idea
> I'm not sure adding another legacy option makes sense - at least the
> option should have the desired -msimd=iwmmxt spelling.

The -mwmmxt option is not acceptable as it stands today.  IIRC the msimd
  option was the plan long term when we talked about this last. It is a
good idea to revisit this now that we are finalizing the options /
multilib rework and the iwmmx port is getting some maintenance.

It would be preferable to no longer have to perpetuate the march=iwmxxt
and mcpu=iwmmxt options if we can avoid it since they really aren't
architectures or cpu's in the traditional sense.  Joseph raises a valid
point when he asks about the interaction with the current command line
options.

There is a lot of restructuring of pattern names in neon.md. When you
say you tested arm-linux-gnueabi did you specifically test the neon port
with your patches applied to be sure that nothing broke there since I
notice this churn ?

Based on a quick skim of the patch -

In a number of places I noticed that you have

For e.g. in your pipeline descriptions .

ior (eq_attr ("wtype" "waligni")
     ior (eq_attr ("wtype" "walignr"))

etc...

You could rationalize these with


eq_attr "wtype" "waligni, walignr" makes these things smaller :)




Also based on a quick reading I find that

1. The documentation for the new intrinsics added is missing and that
needs to be contributed along with the documentation to invoke.texi
about the new options that are being added.
2. EABI build Attribute tags for wmmx2 aren't being added to the
assembler output (maybe the assembler and readelf need to be taught
about this). Look at the Addenda to the ELF ABI about attribute tags.

The patch as it stands today is hard to review - can you break the patch
into smaller chunks that will help review ?

You could have a patch series roughly that does ( I haven't thought
about it much since I haven't read your patch in great detail.)

command line options changes
target addressing changes
one patch that reworks the neon backend and adds the new features of the
vectorization bits of the iwmmxt .md
a patch for the pipeline descriptions .
documentation

This allows the mechanical portions of your patch to be reviewed quicker
than the other bits and potentially by more than one reviewer.


cheers
Ramana