RE: [PATCH][GCC][Arm] Fix subreg crash in different way by enabling the FP16 pattern unconditionally.

[Tamar Christina <Tamar dot Christina at arm dot com>]

Hi Thomas,

Thanks for the review!

> >
> > I don't believe the TARGET_FP16 guard to be needed, because the
> > pattern doesn't actually generate code and requires another pattern
> > for that, and a reg to reg move should always be possible anyway. So
> > allowing the force to register here is safe and it allows the compiler
> > to generate a correct error instead of ICEing in an infinite loop.
> 
> How about subreg to subreg move? Doesn't that expand to more insns
> (subreg to reg and reg to subreg)? Couldn't you improve the logic to check
> that there is actually a mode change so that if there isn't (like moving from
> one subreg to another) just expand to a single move?
> 

Yes, but that is not a new issue. My patch is simply removing the TARGET_FP16 restrictions and
merging two patterns that should be one using an iterator and nothing more.

The redundant mov is already there and a different issue than the ICE I'm trying to fix.

None of the code inside the expander is needed at all, the code really only has an effect on subreg
to subreg moves, as `force_reg` doesn't do anything when it's argument is already a reg.

The comment in the expander (which was already there) is wrong. The *reason* the ICE is fixed isn't
because of the `force_reg`. It's because of the mere presence of the expander itself. The expander matches the
standard mov$a optab and so this prevents emit_move_insn_1 from doing the move by subwords as it finds a pattern
that's able to do the move.

The expander however always falls through and doesn’t stop RTL generation. You could remove all the code in there and have
it properly match the *neon_mov instructions which will do the right thing later at code generation time and avoid the redundant
moves.  My guess is the original `force_reg` was copied from the other patterns like `movti` and the existing `mov<mode>`. There It makes
sense because the operands can be MEM or anything general_operand.

However the redundant moves are a different problem than what I'm trying to solve here. So I think that's another patch which requires further
testing.

Regards,
Tamar

> Best regards,
> 
> Thomas
> 
> >
> > This patch ensures gcc.target/arm/big-endian-subreg.c is fixed without
> > introducing any regressions while fixing
> >
> > gcc.dg/vect/vect-nop-move.c execution test
> > g++.dg/torture/vshuf-v2si.C   -O3 -g  execution test
> > g++.dg/torture/vshuf-v4si.C   -O3 -g  execution test
> > g++.dg/torture/vshuf-v8hi.C   -O3 -g  execution test
> >
> > Regtested on armeb-none-eabi and no regressions.
> > Bootstrapped on arm-none-linux-gnueabihf and no issues.
> >
> >
> > Ok for trunk?
> >
> > Thanks,
> > Tamar
> >
> > gcc/
> > 2018-07-23  Tamar Christina  <tamar.christina@arm.com>
> >
> >         PR target/84711
> >         * config/arm/arm.c (arm_can_change_mode_class): Disallow subreg.
> >         * config/arm/neon.md (movv4hf, movv8hf): Refactored to..
> >         (mov<mov>): ..this and enable unconditionally.
> >
> > --