Fix RTL patterns for NEON vmovn

Hi,

This patch contains a few fixes (likely latent bugs) for the RTL
instruction patterns implementing NEON vmovn support, as added by:

http://gcc.gnu.org/ml/gcc-patches/2010-08/msg02146.html

The fixes are as follows: firstly, the usage of match_dup in the
neon_move_lo_quad_<mode> and neon_move_hi_quad_<mode> is incorrect.
According to the manual, the proper way to implement this type of
operation (modifying a read/write operand) is to use matching
constraints, i.e.:

http://gcc.gnu.org/onlinedocs/gccint/RTL-Template.html#index-match_005fdup-3215

Incidentally this incorrect RTL usage showed up as a wrong-code bug in
the testsuite when backporting vmovn support to our 4.5-based tree,
though I haven't observed it happening with current mainline.
Interestingly or not, on 4.5 the tests which failed first went wrong
because web.c rewrote the destination operand 0 (but not the match_dup'd
source version of same) to a different register for a
neon_move_hi_quad insn. The patch from Bernd Schmitt here:

http://gcc.gnu.org/ml/gcc-patches/2010-03/msg01160.html

appeared sufficient to mask the problem on 4.5, and its presence on
mainline is possibly why the incorrect RTL in the above-named patterns
hasn't shown up problems on the latter so far.

The second and third problems fixed by the patch are incorrect logic to
detect redundant move instructions in neon_move_hi_quad ("dest +
2..."), and a missing length specifier in the
quad-word version of vec_pack_trunc_<mode>.

Tested OK with cross to ARM Linux (-march=armv7-a -mfloat-abi=softfp
-mfpu=neon). OK to apply?

Thanks,

Julian

ChangeLog

    gcc/
    * config/arm/neon.md (neon_move_lo_quad_<mode>): Use matching
    constraint instead of match_dup.
    (neon_move_hi_quad_<mode>): Likewise. Fix condition for omitting
    redundant vmov instruction.
    (move_hi_quad_<mode>): Add extra argument to
    gen_neon_move_hi_quad_<mode> call.
    (move_lo_quad_<mode>): Similar, for gen_neon_move_lo_quad_<mode>
    call.
    (vec_pack_trunc_<mode>): Fix assembly template, add length
    specifier.

On 10/22/2010 03:12 PM, Julian Brown wrote:
> The fixes are as follows: firstly, the usage of match_dup in the
> neon_move_lo_quad_<mode> and neon_move_hi_quad_<mode> is incorrect.
> According to the manual, the proper way to implement this type of
> operation (modifying a read/write operand) is to use matching
> constraints, i.e.:
> 
> http://gcc.gnu.org/onlinedocs/gccint/RTL-Template.html#index-match_005fdup-3215

I think the docs are misleading here.  Nothing wrong with using
match_dup IMO, except slightly less freedom for the compiler to modify
the operands during optimization.  See the discussion that followed the
web.c patch you quoted.


Bernd

On Fri, 22 Oct 2010 15:22:18 +0200
Bernd Schmidt <bernds@codesourcery.com> wrote:

> On 10/22/2010 03:12 PM, Julian Brown wrote:
> > The fixes are as follows: firstly, the usage of match_dup in the
> > neon_move_lo_quad_<mode> and neon_move_hi_quad_<mode> is incorrect.
> > According to the manual, the proper way to implement this type of
> > operation (modifying a read/write operand) is to use matching
> > constraints, i.e.:
> > 
> > http://gcc.gnu.org/onlinedocs/gccint/RTL-Template.html#index-match_005fdup-3215
> 
> I think the docs are misleading here.  Nothing wrong with using
> match_dup IMO, except slightly less freedom for the compiler to modify
> the operands during optimization.  See the discussion that followed
> the web.c patch you quoted.

To clarify: would you expect using matching constraints rather than
match_dup to still provide some improvement (slightly more freedom
for optimizers to modify operands) and thus still be a useful change?
If not, I could commit the other fixes without that fragment.

Cheers,

Julian