[PATCH, ARM] iWMMXT maintenance

[PATCH, ARM] iWMMXT maintenance

[Xinyu Qi]

[-- Attachment #1: Type: text/plain, Size: 1534 bytes --]

Hi,

This patch maintains iWMMXT intrinsics code,
adds WMMX pipeline description
and supports iWMMXT auto-vectorization.
Ran Arm testsuite on arm-linux-gnueabi.

*gcc/config/arm/elf.h: Add option -mwmmxt.
*gcc/config/arm/arm.opt: Same.
*gcc/config/arm/arm.c (arm_option_override): Same.
 (arm_coproc_mem_operand2): Add/fix iWMMXT/iWMMXT2 intrinsic.
 (enum arm_builtins): Same.
 (builtin_description bdesc_2arg): Same.
 (builtin_description bdesc_1arg): Same.
 (arm_init_iwmmxt_builtins): Same.
 (arm_expand_binop_builtin): Same.
 (arm_expand_builtin): Same.
 (arm_output_load_gr): Same.
 (arm_output_iwmmxt_shift_immediate): New function. Same.
 (arm_output_iwmmxt_tinsr): New function. Same.
 (iwmmxt_expand_vector_init): New function. Serving for iWMMXT auto-vectorization.
 (iwmmxt_expand_vector_mov): New function. Same.
*gcc/config/arm/mmintrin.h: Add/fix iWMMXT/iWMMXT2 intrinsic.
*gcc/config/arm/constraints.md: Same.
*gcc/config/arm/predicates.md: Same.
*gcc/config/arm/iterators.md: Same.
*gcc/config/arm/iwmmxt.md: Same.
*gcc/config/arm/iwmmxt2.md: New file. Same.
*gcc/config/arm/arm-protos.h: Add new functions protos.
*gcc/config/arm/marvell-f-iwmmxt.md: New file. 
 Add Marvell WMMX pipeline description.
*gcc/config/arm/arm.md: Fix iWMMXT/iWMMXT2 intrinsic. 
 Add wtype for Marvell WMMX pipeline description. 
*gcc/config/arm/iwmmxt-autovec.md: New file. Support iWMMXT auto-vectorization.
*gcc/config/arm/vec-common.md: Make iWMMXT and NEON co-exist.
*gcc/config/arm/neon.md: Same.

[-- Attachment #2: iWMMXT_maintenance.patch.gz --]
[-- Type: application/x-gzip, Size: 35679 bytes --]

Re: [PATCH, ARM] iWMMXT maintenance

[Joseph S. Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH, ARM] iWMMXT maintenance

[Ramana Radhakrishnan]

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

RE: [PATCH, ARM] iWMMXT maintenance

[Xinyu Qi]

Hi, Ramana and Joseph,

Thank you for your reviewing! Sorry for the late response.
Before I submit the new modified patch, I want to make something more specific.

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

So I decide to remove the option from my patch.
I plan to submit three patches this time, one for iWMMXt intrinsic maintenance and WMMX pipeline description (no auto-vectorization or address fix containing), another for iWMMXt testsuite, and the third for doc update.
Do you think it's better to split iWMMXt intrinsic maintenance and WMMX pipeline description into two patches? 

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

About the documentation, I found there is no iWMMXt intrinsic doc in extend.texi (which only has WMMX built-in function doc instead).With reference to NEON (existing NEON intrinsic doc), should the iWMMXt intrinsic doc be provide or just simply update the WMMX build-in function? Is it possible to postpone the doc patch since it maybe takes a long time to prepare?

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

I have tested all the neon test under gcc.target/arm and gcc.target/arm/neon. I prefer holding the WMMX auto-vectorization patch for a while.

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

Thanks for direction! That's really convenient.

Thanks,
Xinyu