* [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830]
@ 2021-04-09 7:49 Jakub Jelinek
2021-04-10 6:02 ` [PATCH] combine: Don't fold away side-effects in simplify_and_const_int_1 [PR99830] Jakub Jelinek
2021-04-12 22:49 ` [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] Segher Boessenkool
0 siblings, 2 replies; 4+ messages in thread
From: Jakub Jelinek @ 2021-04-09 7:49 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
Hi!
On the following testcase on aarch64 the combiner propagates
(clobber:TI (const_int 0)) into a DEBUG_INSN. Such clobbers are
specific to the combiner, created by gen_lowpart_for_combine:
fail:
return gen_rtx_CLOBBER (omode, const0_rtx);
which can be embedded anywhere and the combiner hopes they never match
anything. That is hopefully the case of all instructions that go through
recog, but for DEBUG_INSNs we don't have any strict rules on what matches
and what doesn't and so if combiner calls propagate_for_debug with
src containing such clobbers embedded in it, it will happily propagate it
into DEBUG_INSNs and cause problems later (in this case ICEs during LRA).
The following patch ensures that if such clobbers would be propagated into
some DEBUG_INSNs that such DEBUG_INSNs are reset to the unknown state.
propagate_for_debug uses volatile_insn_p check to determine if something
needs to be reset after propagation and the patch replaces the combiner
specific clobbers with UNSPEC_VOLATILE that volatile_insn_p will return
true on and thus valtrack.c reset those DEBUG_INSNs.
Bootstrapped/regtested on x86_64-linux and i686-linux and tested in
cross to aarch64-linux on the testcase, ok for trunk?
2021-04-09 Jakub Jelinek <jakub@redhat.com>
PR debug/99830
* combine.c (combine_propagate_for_debug): New function.
(try_combine): Use combine_propagate_for_debug instead of
propagate_for_debug.
* gcc.dg/pr99830.c: New test.
--- gcc/combine.c.jj 2021-04-07 12:35:03.060224972 +0200
+++ gcc/combine.c 2021-04-08 16:22:17.851459742 +0200
@@ -2610,6 +2610,30 @@ count_auto_inc (rtx, rtx, rtx, rtx, rtx,
return 0;
}
+/* Wrapper around propagate_for_debug to deal with gen_lowpart_for_combine
+ (clobber (const_int 0)). In normal insns a clobber of const0_rtx nested
+ somewhere will cause the pattern not to be recognized, but in debug insns
+ it doesn't, one needs to use something on which volatile_insn_p is true
+ instead. */
+
+static void
+combine_propagate_for_debug (rtx_insn *insn, rtx_insn *last, rtx dest, rtx src,
+ basic_block this_basic_block)
+{
+ subrtx_iterator::array_type array;
+ FOR_EACH_SUBRTX (iter, array, src, ALL)
+ {
+ const_rtx x = *iter;
+ if (GET_CODE (x) == CLOBBER && XEXP (x, 0) == const0_rtx)
+ {
+ src = gen_rtx_UNSPEC_VOLATILE (GET_MODE (dest),
+ gen_rtvec (1, const0_rtx), -1);
+ break;
+ }
+ }
+ propagate_for_debug (insn, last, dest, src, this_basic_block);
+}
+
/* Try to combine the insns I0, I1 and I2 into I3.
Here I0, I1 and I2 appear earlier than I3.
I0 and I1 can be zero; then we combine just I2 into I3, or I1 and I2 into
@@ -4200,8 +4224,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
i2src while its original mode is temporarily
restored, and then clear i2scratch so that we don't
do it again later. */
- propagate_for_debug (i2, last_combined_insn, reg, i2src,
- this_basic_block);
+ combine_propagate_for_debug (i2, last_combined_insn, reg,
+ i2src, this_basic_block);
i2scratch = false;
/* Put back the new mode. */
adjust_reg_mode (reg, new_mode);
@@ -4235,12 +4259,13 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
with this copy we have created; then, replace the
copy with the SUBREG of the original shared reg,
once again changed to the new mode. */
- propagate_for_debug (first, last, reg, tempreg,
- this_basic_block);
+ combine_propagate_for_debug (first, last, reg, tempreg,
+ this_basic_block);
adjust_reg_mode (reg, new_mode);
- propagate_for_debug (first, last, tempreg,
- lowpart_subreg (old_mode, reg, new_mode),
- this_basic_block);
+ combine_propagate_for_debug (first, last, tempreg,
+ lowpart_subreg (old_mode, reg,
+ new_mode),
+ this_basic_block);
}
}
}
@@ -4499,16 +4524,16 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
if (newi2pat)
{
if (MAY_HAVE_DEBUG_BIND_INSNS && i2scratch)
- propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
- this_basic_block);
+ combine_propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
+ this_basic_block);
INSN_CODE (i2) = i2_code_number;
PATTERN (i2) = newi2pat;
}
else
{
if (MAY_HAVE_DEBUG_BIND_INSNS && i2src)
- propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
- this_basic_block);
+ combine_propagate_for_debug (i2, last_combined_insn, i2dest, i2src,
+ this_basic_block);
SET_INSN_DELETED (i2);
}
@@ -4517,8 +4542,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
LOG_LINKS (i1) = NULL;
REG_NOTES (i1) = 0;
if (MAY_HAVE_DEBUG_BIND_INSNS)
- propagate_for_debug (i1, last_combined_insn, i1dest, i1src,
- this_basic_block);
+ combine_propagate_for_debug (i1, last_combined_insn, i1dest, i1src,
+ this_basic_block);
SET_INSN_DELETED (i1);
}
@@ -4527,8 +4552,8 @@ try_combine (rtx_insn *i3, rtx_insn *i2,
LOG_LINKS (i0) = NULL;
REG_NOTES (i0) = 0;
if (MAY_HAVE_DEBUG_BIND_INSNS)
- propagate_for_debug (i0, last_combined_insn, i0dest, i0src,
- this_basic_block);
+ combine_propagate_for_debug (i0, last_combined_insn, i0dest, i0src,
+ this_basic_block);
SET_INSN_DELETED (i0);
}
--- gcc/testsuite/gcc.dg/pr99830.c.jj 2021-04-08 16:39:44.243774333 +0200
+++ gcc/testsuite/gcc.dg/pr99830.c 2021-04-08 16:28:40.463175738 +0200
@@ -0,0 +1,10 @@
+/* PR debug/99830 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -fno-expensive-optimizations -fno-split-wide-types -g" } */
+
+int foo (long a, __int128 b, short c, int d, unsigned e, __int128 f)
+{
+ __builtin_memmove (2 + (char *) &f, foo, 1);
+ c >>= (char) f;
+ return c;
+}
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] combine: Don't fold away side-effects in simplify_and_const_int_1 [PR99830]
2021-04-09 7:49 [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] Jakub Jelinek
@ 2021-04-10 6:02 ` Jakub Jelinek
2021-04-12 22:53 ` Segher Boessenkool
2021-04-12 22:49 ` [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] Segher Boessenkool
1 sibling, 1 reply; 4+ messages in thread
From: Jakub Jelinek @ 2021-04-10 6:02 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
Hi!
Here is an alternate patch for the PR99830 bug.
As discussed on IRC and in the PR, the reason why a (clobber:TI (const_int 0))
has been propagated into the debug insns is that it got optimized away
during simplification from the i3 instruction pattern.
And that happened because
simplify_and_const_int_1 (SImode, varop, 255)
with varop of
(ashift:SI (subreg:SI (and:TI (clobber:TI (const_int 0 [0]))
(const_int 255 [0xff])) 0)
(const_int 16 [0x10]))
was called and through nonzero_bits determined that (whatever << 16) & 255
is const0_rtx.
It is, but if there are side-effects in varop and such clobbers are
considered as such, we shouldn't optimize those away.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2021-04-10 Jakub Jelinek <jakub@redhat.com>
PR debug/99830
* combine.c (simplify_and_const_int_1): Don't optimize varop
away if it has side-effects.
* gcc.dg/pr99830.c: New test.
--- gcc/combine.c.jj 2021-04-08 17:01:37.315168182 +0200
+++ gcc/combine.c 2021-04-09 22:33:56.961264092 +0200
@@ -10153,7 +10153,7 @@ simplify_and_const_int_1 (scalar_int_mod
constop &= nonzero;
/* If we don't have any bits left, return zero. */
- if (constop == 0)
+ if (constop == 0 && !side_effects_p (varop))
return const0_rtx;
/* If VAROP is a NEG of something known to be zero or 1 and CONSTOP is
--- gcc/testsuite/gcc.dg/pr99830.c.jj 2021-04-08 16:39:44.243774333 +0200
+++ gcc/testsuite/gcc.dg/pr99830.c 2021-04-08 16:28:40.463175738 +0200
@@ -0,0 +1,10 @@
+/* PR debug/99830 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -fno-expensive-optimizations -fno-split-wide-types -g" } */
+
+int foo (long a, __int128 b, short c, int d, unsigned e, __int128 f)
+{
+ __builtin_memmove (2 + (char *) &f, foo, 1);
+ c >>= (char) f;
+ return c;
+}
Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830]
2021-04-09 7:49 [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] Jakub Jelinek
2021-04-10 6:02 ` [PATCH] combine: Don't fold away side-effects in simplify_and_const_int_1 [PR99830] Jakub Jelinek
@ 2021-04-12 22:49 ` Segher Boessenkool
1 sibling, 0 replies; 4+ messages in thread
From: Segher Boessenkool @ 2021-04-12 22:49 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
Hi!
On Fri, Apr 09, 2021 at 09:49:30AM +0200, Jakub Jelinek wrote:
> On the following testcase on aarch64 the combiner propagates
> (clobber:TI (const_int 0)) into a DEBUG_INSN. Such clobbers are
> specific to the combiner, created by gen_lowpart_for_combine:
> fail:
> return gen_rtx_CLOBBER (omode, const0_rtx);
> which can be embedded anywhere and the combiner hopes they never match
> anything.
It doesn't just hope that, there is code all over the place that
guarantees this is true. Yes, that isn't super robust, I agree. But
that is what we have until someone manages to change it.
> That is hopefully the case of all instructions that go through
> recog, but for DEBUG_INSNs we don't have any strict rules on what matches
> and what doesn't and so if combiner calls propagate_for_debug with
> src containing such clobbers embedded in it, it will happily propagate it
> into DEBUG_INSNs and cause problems later (in this case ICEs during LRA).
It should never get that far.
(We talked about this; just adding it here to archive it for posterity.)
Segher
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] combine: Don't fold away side-effects in simplify_and_const_int_1 [PR99830]
2021-04-10 6:02 ` [PATCH] combine: Don't fold away side-effects in simplify_and_const_int_1 [PR99830] Jakub Jelinek
@ 2021-04-12 22:53 ` Segher Boessenkool
0 siblings, 0 replies; 4+ messages in thread
From: Segher Boessenkool @ 2021-04-12 22:53 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
Hi!
On Sat, Apr 10, 2021 at 08:02:29AM +0200, Jakub Jelinek wrote:
> Here is an alternate patch for the PR99830 bug.
> As discussed on IRC and in the PR, the reason why a (clobber:TI (const_int 0))
> has been propagated into the debug insns is that it got optimized away
> during simplification from the i3 instruction pattern.
>
> And that happened because
> simplify_and_const_int_1 (SImode, varop, 255)
> with varop of
> (ashift:SI (subreg:SI (and:TI (clobber:TI (const_int 0 [0]))
> (const_int 255 [0xff])) 0)
> (const_int 16 [0x10]))
> was called and through nonzero_bits determined that (whatever << 16) & 255
> is const0_rtx.
> It is, but if there are side-effects in varop and such clobbers are
> considered as such, we shouldn't optimize those away.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Okay for trunk. Thank you!
We'll need to audit all other code doing similar things as well, or add
some assert at some strategic spot. Something for GCC 12 :-)
Segher
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-12 22:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09 7:49 [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] Jakub Jelinek
2021-04-10 6:02 ` [PATCH] combine: Don't fold away side-effects in simplify_and_const_int_1 [PR99830] Jakub Jelinek
2021-04-12 22:53 ` Segher Boessenkool
2021-04-12 22:49 ` [PATCH] combine: Avoid propagation of (clobber (const_int 0)) into DEBUG_INSNs [PR99830] Segher Boessenkool
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).