Hi Kyrill, Many thanks for the review!
The 06/20/2018 09:43, Kyrill Tkachov wrote: > 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.christ...@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.christ...@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. I don't know what the documentation is trying to say here, but the key is the first part: "returns the size in bytes of the subunits of a datum of mode m" MODE: V4SI, GET_MODE_UNIT_SIZE: 4, GET_MODE_SIZE: 16 So GET_MODE_UNIT_SIZE (m) * GET_MODE_NUNITS(m) == GET_MODE_SIZE (m) or GET_MODE_UNIT_SIZE (m) == GET_MODE_SIZE (GET_MODE_INNER (mode)). >From this the only time GET_MODE_UNIT_SIZE is equal to GET_MODE_SIZE is on non-vector modes of V1 vector modes. Kind Regards, Tamar > > Thanks for the patch, > Kyrill > > > --
