PR 69577: Invalid RA of destination subregs
Richard Sandiford
richard.sandiford@arm.com
Tue Feb 2 17:57:00 GMT 2016
[Resending without disclaimer, sorry]
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.
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;
+}
More information about the Gcc-patches
mailing list