Re: [PATCH][GCC][ARM] Fix can_change_mode_class for big-endian

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Tamar,

On 19/06/18 15:14, Tamar Christina wrote:
> Hi All,
>
> This patch requires https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01145.html to work,
> it has been accepted once already but caused a regression on certain configuratoins.
> I am re-submitting it with the required mid-end change and requesting a back-port.
>
> --- original patch.
>
> Taking the subreg of a vector mode on big-endian may result in an infinite
> recursion and eventually a segfault once we run out of stack space.
>
> As an example, taking a subreg of V4HF to SImode we end up in the following
> loop on big-endian:
>
> #861 0x00000000008462e9 in operand_subword_force src/gcc/gcc/emit-rtl.c:1787
> #862 0x0000000000882a90 in emit_move_multi_word src/gcc/gcc/expr.c:3621
> #863 0x000000000087eea1 in emit_move_insn_1 src/gcc/gcc/expr.c:3698
> #864 0x000000000087f350 in emit_move_insn src/gcc/gcc/expr.c:3757
> #865 0x000000000085e326 in copy_to_reg src/gcc/gcc/explow.c:603
> #866 0x00000000008462e9 in operand_subword_force src/gcc/gcc/emit-rtl.c:1787
>
> The reason is that operand_subword_force will always fail. When the value is in
> a register that can't be accessed as a multi word the code tries to create a new
> psuedo register and emit the value to it. Eventually you end up in simplify_gen_subreg
> which calls validate_subreg.
>
> validate_subreg will however always fail because of the REG_CAN_CHANGE_MODE_P check.
>
> On little endian this check always returns true. On big-endian this check is supposed
> to prevent values that have a size larger than word size, due to those being stored in
> VFP registers.
>
> However we are only interested in a subreg of the vector mode, so we should be checking
> the unit size, not the size of the entire mode. Doing this fixes the problem.
>
> Regtested on armeb-none-eabi and no regressions.
> Bootstrapped on arm-none-linux-gnueabihf and no issues.
>
> Ok for trunk? and for backport to GCC 8?
>
> Thanks,
> Tamar
>
> gcc/
> 2018-06-19  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/84711
>         * config/arm/arm.c (arm_can_change_mode_class): Use GET_MODE_UNIT_SIZE
>         instead of GET_MODE_SIZE when comparing Units.
>
> gcc/testsuite/
> 2018-06-19  Tamar Christina  <tamar.christina@arm.com>
>
>         PR target/84711
>         * gcc.target/arm/big-endian-subreg.c: New.
>
> --


diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 90d62e699bce9594879be2e3016c9b36c7e064c8..703632240822e762a906570964b949c783df56f3 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -31508,8 +31508,8 @@ arm_can_change_mode_class (machine_mode from, machine_mode to,
 {
   if (TARGET_BIG_END
       && !(GET_MODE_SIZE (from) == 16 && GET_MODE_SIZE (to) == 8)
-      && (GET_MODE_SIZE (from) > UNITS_PER_WORD
-	  || GET_MODE_SIZE (to) > UNITS_PER_WORD)
+      && (GET_MODE_UNIT_SIZE (from) > UNITS_PER_WORD
+	  || GET_MODE_UNIT_SIZE (to) > UNITS_PER_WORD)

Does GET_MODE_UNIT_SIZE do what you want? Its documentation in rtl.texi says:
"Returns the size in bytes of the subunits of a datum of mode @var{m}.
 This is the same as @code{GET_MODE_SIZE} except in the case of complex
 modes.  For them, the unit size is the size of the real or imaginary
 part."

Does it also do the right thing for vector modes (GET_MODE_SIZE (GET_MODE_INNER (mode))) ?
If so, this patch is ok, but we'll need to update the documentation to make it more explicit.

Thanks for the patch,
Kyrill