Re: PR 69577: Invalid RA of destination subregs

[Richard Sandiford <richard dot sandiford at arm dot com>]

Richard Sandiford <richard.sandiford@arm.com> writes:
> Uros Bizjak <ubizjak@gmail.com> writes:
>> On Tue, Feb 2, 2016 at 5:54 PM, Kyrill Tkachov
>> <kyrylo.tkachov@foss.arm.com> wrote:
>>> Hi Richard,
>>>
>>>
>>> On 02/02/16 14:56, Richard Sandiford wrote:
>>>>
>>>> In PR 69577 we have:
>>>>
>>>>    A: (set (reg:V2TI X) ...)
>>>>    B: (set (subreg:TI (reg:V2TI X) 0) ...)
>>>>
>>>> X gets allocated to an AVX register, as usual for V2TI.  The problem is
>>>> that the movti for B doesn't then preserve the other half of X, even
>>>> though the subreg semantics are supposed to guarantee that.
>>>>
>>>> If instead the same value had been set by:
>>>>
>>>>    A': (set (subreg:TI (reg:V2TI X) 16) ...)
>>>>    B: (set (subreg:TI (reg:V2TI X) 0) ...)
>>>>
>>>> the subreg in A' would have prevented the use of AVX registers for X,
>>>> since you can't directly access the high part.
>>>>
>>>> IMO these are really the same thing.  An alternative way to view it
>>>> is that the original sequence is equivalent to:
>>>>
>>>>    A: (set (reg:V2TI X) ...)
>>>>    B1: (set (subreg:TI (reg:V2TI X) 0) ...)
>>>>    B2: (set (subreg:TI (reg:V2TI X) 16) (subreg:TI (reg:V2TI X) 16))
>>>>
>>>> in which B2 is a no-op and therefore implicit.  The handling ought
>>>> to be the same regardless of whether there is an rtl insn that
>>>> explicitly assigns to (subreg:TI (reg:V2TI X) 16).
>>>>
>>>> This patch implements that idea.  Hopefully the comments explain
>>>> what's going on.
>>>>
>>>> Tested on x86_64-linux-gnu so far.  Will test on aarch64-linux-gnu and
>>>> arm-linux-gnueabihf as well.  OK to install if the additional testing
>>>> succeeds?
>>>
>>>
>>> For me this patch causes an ICE when building libgcc during an
>>> aarch64-none-elf build.
>>> It's a segfault with the trace:
>>> 0xb0ac2a crash_signal
>>>         $SRC/gcc/toplev.c:335
>>> 0xa7cfd7 init_subregs_of_mode()
>>>         $SRC/gcc/reginfo.c:1345
>>> 0x96fc4b init_costs
>>>         $SRC/gcc/ira-costs.c:2187
>>> 0x97419e ira_set_pseudo_classes(bool, _IO_FILE*)
>>>         $SRC/gcc/ira-costs.c:2237
>>> 0x106fd1e alloc_global_sched_pressure_data
>>>         $SRC/gcc/haifa-sched.c:7244
>>> 0x106fd1e sched_init()
>>>         $SRC/gcc/haifa-sched.c:7394
>>> 0x107109a haifa_sched_init()
>>>         $SRC/gcc/haifa-sched.c:7406
>>> 0xab37ac schedule_insns()
>>>         $SRC/gcc/sched-rgn.c:3504
>>> 0xab3f5b rest_of_handle_sched
>>>         $SRC/gcc/sched-rgn.c:3717
>>> 0xab3f5b execute
>>>         $SRC/gcc/sched-rgn.c:3825
>>
>> Also on x86_64-linux-gnu when building -m32 multilib:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x0000000000d28264 in init_subregs_of_mode () at
>> /home/uros/gcc-svn/trunk/gcc/reginfo.c:1345
>> 1345            FOR_EACH_INSN_DEF (def, insn)
>> (gdb) p insn
>> $1 = (rtx_insn *) 0x7fffef9f4d40
>> (gdb) p debug_rtx (insn)
>> (code_label 60 31 39 10 9 "" [3 uses])
>> $2 = void
>> (gdb) p def
>> $3 = (df_ref) 0x0
>
> Bah, sorry.  I test with --enable-checking=yes,rtl,df, and it turns out
> that df checking masks this kind of problem.  -m32 builds (and tests)
> fine with it but not without.
>
> Here's the patch again with the obvious fix.  Retesting now with just
> --enable-checking=yes,rtl.

Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabihf.
OK to install?

Thanks,
Richard

> gcc/
> 	PR rtl-optimization/69577
> 	* reginfo.c (record_subregs_of_mode): Add a partial_def parameter.
> 	(find_subregs_of_mode): Update accordingly.  Iterate over partial
> 	definitions.
>
> gcc/testsuite/
> 	PR rtl-optimization/69577
> 	* gcc.target/i386/pr69577.c: New test.
>
> diff --git a/gcc/reginfo.c b/gcc/reginfo.c
> index 6814eed..ccf53bf 100644
> --- a/gcc/reginfo.c
> +++ b/gcc/reginfo.c
> @@ -1244,8 +1244,16 @@ simplifiable_subregs (const subreg_shape &shape)
>  static HARD_REG_SET **valid_mode_changes;
>  static obstack valid_mode_changes_obstack;
>  
> +/* Restrict the choice of register for SUBREG_REG (SUBREG) based
> +   on information about SUBREG.
> +
> +   If PARTIAL_DEF, SUBREG is a partial definition of a multipart inner
> +   register and we want to ensure that the other parts of the inner
> +   register are correctly preserved.  If !PARTIAL_DEF we need to
> +   ensure that SUBREG itself can be formed.  */
> +
>  static void
> -record_subregs_of_mode (rtx subreg)
> +record_subregs_of_mode (rtx subreg, bool partial_def)
>  {
>    unsigned int regno;
>  
> @@ -1256,15 +1264,41 @@ record_subregs_of_mode (rtx subreg)
>    if (regno < FIRST_PSEUDO_REGISTER)
>      return;
>  
> +  subreg_shape shape (shape_of_subreg (subreg));
> +  if (partial_def)
> +    {
> +      /* The number of independently-accessible SHAPE.outer_mode values
> +	 in SHAPE.inner_mode is GET_MODE_SIZE (SHAPE.inner_mode) / SIZE.
> +	 We need to check that the assignment will preserve all the other
> +	 SIZE-byte chunks in the inner register besides the one that
> +	 includes SUBREG.
> +
> +	 In practice it is enough to check whether an equivalent
> +	 SHAPE.inner_mode value in an adjacent SIZE-byte chunk can be formed.
> +	 If the underlying registers are small enough, both subregs will
> +	 be valid.  If the underlying registers are too large, one of the
> +	 subregs will be invalid.
> +
> +	 This relies on the fact that we've already been passed
> +	 SUBREG with PARTIAL_DEF set to false.  */
> +      unsigned int size = MAX (REGMODE_NATURAL_SIZE (shape.inner_mode),
> +			       GET_MODE_SIZE (shape.outer_mode));
> +      gcc_checking_assert (size < GET_MODE_SIZE (shape.inner_mode));
> +      if (shape.offset >= size)
> +	shape.offset -= size;
> +      else
> +	shape.offset += size;
> +    }
> +
>    if (valid_mode_changes[regno])
>      AND_HARD_REG_SET (*valid_mode_changes[regno],
> -		      simplifiable_subregs (shape_of_subreg (subreg)));
> +		      simplifiable_subregs (shape));
>    else
>      {
>        valid_mode_changes[regno]
>  	= XOBNEW (&valid_mode_changes_obstack, HARD_REG_SET);
>        COPY_HARD_REG_SET (*valid_mode_changes[regno],
> -			 simplifiable_subregs (shape_of_subreg (subreg)));
> +			 simplifiable_subregs (shape));
>      }
>  }
>  
> @@ -1277,7 +1311,7 @@ find_subregs_of_mode (rtx x)
>    int i;
>  
>    if (code == SUBREG)
> -    record_subregs_of_mode (x);
> +    record_subregs_of_mode (x, false);
>  
>    /* Time for some deep diving.  */
>    for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
> @@ -1305,7 +1339,14 @@ init_subregs_of_mode (void)
>    FOR_EACH_BB_FN (bb, cfun)
>      FOR_BB_INSNS (bb, insn)
>        if (NONDEBUG_INSN_P (insn))
> -        find_subregs_of_mode (PATTERN (insn));
> +	{
> +	  find_subregs_of_mode (PATTERN (insn));
> +	  df_ref def;
> +	  FOR_EACH_INSN_DEF (def, insn)
> +	    if (DF_REF_FLAGS_IS_SET (def, DF_REF_PARTIAL)
> +		&& df_read_modify_subreg_p (DF_REF_REG (def)))
> +	      record_subregs_of_mode (DF_REF_REG (def), true);
> +	}
>  }
>  
>  const HARD_REG_SET *
> diff --git a/gcc/testsuite/gcc.target/i386/pr69577.c b/gcc/testsuite/gcc.target/i386/pr69577.c
> new file mode 100644
> index 0000000..d680539
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr69577.c
> @@ -0,0 +1,25 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target avx } */
> +/* { dg-require-effective-target int128 } */
> +/* { dg-options "-O -fno-forward-propagate -fno-split-wide-types -mavx" } */
> +
> +typedef unsigned int u32;
> +typedef unsigned __int128 u128;
> +typedef unsigned __int128 v32u128 __attribute__ ((vector_size (32)));
> +
> +u128 __attribute__ ((noinline, noclone))
> +foo (u32 u32_0, v32u128 v32u128_0)
> +{
> +  v32u128_0[0] >>= u32_0;
> +  v32u128_0 += (v32u128) {u32_0, 0};
> +  return u32_0 + v32u128_0[0] + v32u128_0[1];
> +}
> +
> +int
> +main()
> +{
> +  u128 x = foo (1, (v32u128) {1, 4});
> +  if (x != 6)
> +    __builtin_abort ();
> +  return 0;
> +}