Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
On Apr 5, 2011, Jakub Jelinek wrote: > or DF would have resetted those debug_insns referencing it > (or replaced the pseudos in there with a debug temporary set before the last > real use), otherwise debug insns would change the liveness of the pseudo. Ah, right, that was the bit I was missing! (it's unfortunate that SSA and DF are so different in this regard) > But with some previous propagate_for_debug calls in the same bb that > guarantee (as the testcase shows) is no longer true. I see. The patch looks like the right fix, then. Thanks! -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist Red Hat Brazil Compiler Engineer
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
On Tue, Apr 05, 2011 at 02:24:55PM -0300, Alexandre Oliva wrote: > On Apr 4, 2011, Jakub Jelinek wrote: > > > Alternatively, perhaps all we care about is either i3, or NEXT_INSN of > > the last debug_insn propagate_for_debug changed in an interesting way. > > Ideally, we'd like to identify all debug users of i2dest (or whatever > DEF we're removing) and propagate into them, even if they are e.g. in > different blocks or after i3. If they are in different blocks, then either there must be some other use of the pseudo in real code after that (but then combine won't do such replacements), or DF would have resetted those debug_insns referencing it (or replaced the pseudos in there with a debug temporary set before the last real use), otherwise debug insns would change the liveness of the pseudo. > IIRC stopping at i3 is a trade-off that avoids making the analysis more > complex, just because we know i2dest, if removed, isn't used after i3 > any more, although it might still be used in debug insns. I don't > recall how/when(/whether) we adjust those afterwards. The change mode of pseudos propagation can't be a best effort, we really must change all the pseudos (or reset debug_insns containing them), otherwise the RTL in the insns might be invalid. The other changes we propagate are best effort, if we don't change all, and some pseudo remains referenced only in debug insns and not really set in real code or used anywhere, next DF recomputation will just reset them. Of course the less often that happens, the better. > Anyhow, I don't quite understand what makes the retry case special. > Each try_combine call (and its debug propagations) are supposed to be > self-contained. If we dropped a SET, we should adjust all debug uses of > its SET_DEST; if we didn't drop it, no adjustment is needed. It seems > to me that extending the range might even cause us to modify > e.g. unrelated uses of the same pseudo, no? Isn't the problem perhaps > that we are failing to adjust some debug insns at an earlier call? The reason why retry case is special is that without any previous propagate_for_debug calls we know that e.g. in the change mode case, the pseudo can't be referenced before i2 and after i3 where i2 is the single setter of the pseudo and i3 is the single (non-debug) user. Otherwise RTL would not be valid before combiner. But with some previous propagate_for_debug calls in the same bb that guarantee (as the testcase shows) is no longer true. We might have propagate_for_debug, replacing some i2dest with i2src in between i2 and i3, then goto retry with new i3' being somewhere before previous i3. We change mode of the pseudo that was present in old i2src, but if we stop looking for debug insns at i3', there might be some debug insn in between i3' and i3 that references that pseudo. Jakub
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
On Apr 4, 2011, Jakub Jelinek wrote: > Alternatively, perhaps all we care about is either i3, or NEXT_INSN of > the last debug_insn propagate_for_debug changed in an interesting way. Ideally, we'd like to identify all debug users of i2dest (or whatever DEF we're removing) and propagate into them, even if they are e.g. in different blocks or after i3. IIRC stopping at i3 is a trade-off that avoids making the analysis more complex, just because we know i2dest, if removed, isn't used after i3 any more, although it might still be used in debug insns. I don't recall how/when(/whether) we adjust those afterwards. Anyhow, I don't quite understand what makes the retry case special. Each try_combine call (and its debug propagations) are supposed to be self-contained. If we dropped a SET, we should adjust all debug uses of its SET_DEST; if we didn't drop it, no adjustment is needed. It seems to me that extending the range might even cause us to modify e.g. unrelated uses of the same pseudo, no? Isn't the problem perhaps that we are failing to adjust some debug insns at an earlier call? -- Alexandre Oliva, freedom fighterhttp://FSFLA.org/~lxoliva/ You must be the change you wish to see in the world. -- Gandhi Be Free! -- http://FSFLA.org/ FSF Latin America board member Free Software Evangelist Red Hat Brazil Compiler Engineer
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
> Alternatively, perhaps all we care about is either i3, or NEXT_INSN of the > last debug_insn propagate_for_debug changed in an interesting way. Thus > propagate_for_debug could return the last DEBUG_INSN it changed, and caller > decide either that no updating of last_modified_debug_insn is needed (that > is the case of the pair of propagate_for_debug calls from first to last > which just make it a lowpart subreg), or it would update it. Maybe. I was more or less waiting for Alexandre's take on all this. :-) > 2011-04-04 Jakub Jelinek > > PR debug/48343 > * combine.c (combine_instructions): Add last_combined_insn, > update it if insn is after it, pass it to all try_combine > calls. > (try_combine): Add last_combined_insn parameter, pass it instead of > i3 to propagate_for_debug. > > * gcc.dg/torture/pr48343.c: New test. This version is fine by me at least, thanks. -- Eric Botcazou
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
On Mon, Apr 04, 2011 at 09:55:12AM +0200, Jakub Jelinek wrote: > So something like this? Now bootstrapped/regtested on x86_64-linux and i686-linux, ok? > 2011-04-04 Jakub Jelinek > > PR debug/48343 > * combine.c (combine_instructions): Add last_combined_insn, > update it if insn is after it, pass it to all try_combine > calls. > (try_combine): Add last_combined_insn parameter, pass it instead of > i3 to propagate_for_debug. > > * gcc.dg/torture/pr48343.c: New test. Jakub
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
On Sun, Apr 03, 2011 at 11:39:22AM +0200, Eric Botcazou wrote:
> > I think we need to update there in all cases. The reason we don't need to
> > update beyond i3 resp. undobuf.other_insn is that DF guarantees us that
> > there won't be debug insns referring to those pseudos afterwards, otherwise
> > either the pseudo must be live afterwards in real code (then it wouldn't
> > be a single use case), or debug insns would be reset, or a debug temporary
> > would be created, where the temporary is set before last place where
> > the pseudo is used in real code. Now, once we propagate_for_debug after
> > some insn, DF hasn't been run in between and thus the pseudos might be live
> > afterwards.
>
> Frankly moving down last_combined_insn to undobuf.other_insn in the UNDO_MODE
> case seems a little overengineered at this point.
You are right, in that case we only replace regs with lowpart subreg
thereof, so it shouldn't extend the lifetime of any pseudo.
> > If you just want to avoid a global variable, the code can be surely changed
> > to have a local variable from combine_instructions and pass address to that
> > to all try_combine calls, but other than that I think we should do what the
> > patch does.
>
> I'd eliminate the global variable and directly pass the insn to try_combine,
> this is good enough for now IMO.
So something like this?
Alternatively, perhaps all we care about is either i3, or NEXT_INSN of the last
debug_insn propagate_for_debug changed in an interesting way. Thus
propagate_for_debug could return the last DEBUG_INSN it changed, and caller
decide either that no updating of last_modified_debug_insn is needed (that
is the case of the pair of propagate_for_debug calls from first to last
which just make it a lowpart subreg), or it would update it.
2011-04-04 Jakub Jelinek
PR debug/48343
* combine.c (combine_instructions): Add last_combined_insn,
update it if insn is after it, pass it to all try_combine
calls.
(try_combine): Add last_combined_insn parameter, pass it instead of
i3 to propagate_for_debug.
* gcc.dg/torture/pr48343.c: New test.
--- gcc/combine.c.jj2011-04-04 08:56:08.0 +0200
+++ gcc/combine.c 2011-04-04 09:41:13.0 +0200
@@ -387,7 +387,7 @@ static int cant_combine_insn_p (rtx);
static int can_combine_p (rtx, rtx, rtx, rtx, rtx, rtx, rtx *, rtx *);
static int combinable_i3pat (rtx, rtx *, rtx, rtx, rtx, int, int, rtx *);
static int contains_muldiv (rtx);
-static rtx try_combine (rtx, rtx, rtx, rtx, int *);
+static rtx try_combine (rtx, rtx, rtx, rtx, int *, rtx);
static void undo_all (void);
static void undo_commit (void);
static rtx *find_split_point (rtx *, rtx, bool);
@@ -1159,6 +1159,7 @@ combine_instructions (rtx f, unsigned in
FOR_EACH_BB (this_basic_block)
{
+ rtx last_combined_insn = NULL_RTX;
optimize_this_for_speed_p = optimize_bb_for_speed_p (this_basic_block);
last_call_luid = 0;
mem_last_set = -1;
@@ -1177,6 +1178,10 @@ combine_instructions (rtx f, unsigned in
next = 0;
if (NONDEBUG_INSN_P (insn))
{
+ if (last_combined_insn == NULL_RTX
+ || DF_INSN_LUID (last_combined_insn) < DF_INSN_LUID (insn))
+ last_combined_insn = insn;
+
/* See if we know about function return values before this
insn based upon SUBREG flags. */
check_promoted_subreg (insn, PATTERN (insn));
@@ -1190,7 +1195,8 @@ combine_instructions (rtx f, unsigned in
for (links = LOG_LINKS (insn); links; links = XEXP (links, 1))
if ((next = try_combine (insn, XEXP (links, 0), NULL_RTX,
-NULL_RTX, &new_direct_jump_p)) != 0)
+NULL_RTX, &new_direct_jump_p,
+last_combined_insn)) != 0)
goto retry;
/* Try each sequence of three linked insns ending with this one.
*/
@@ -1208,8 +1214,8 @@ combine_instructions (rtx f, unsigned in
nextlinks;
nextlinks = XEXP (nextlinks, 1))
if ((next = try_combine (insn, link, XEXP (nextlinks, 0),
-NULL_RTX,
-&new_direct_jump_p)) != 0)
+NULL_RTX, &new_direct_jump_p,
+last_combined_insn)) != 0)
goto retry;
}
@@ -1227,14 +1233,15 @@ combine_instructions (rtx f, unsigned in
&& sets_cc0_p (PATTERN (prev)))
{
if ((next = try_combine (insn, prev, NULL_RTX, NULL_RTX,
- &new_direct_jump_p)) != 0)
+ &new_direct_jump_p,
+
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
> I think we need to update there in all cases. The reason we don't need to > update beyond i3 resp. undobuf.other_insn is that DF guarantees us that > there won't be debug insns referring to those pseudos afterwards, otherwise > either the pseudo must be live afterwards in real code (then it wouldn't > be a single use case), or debug insns would be reset, or a debug temporary > would be created, where the temporary is set before last place where > the pseudo is used in real code. Now, once we propagate_for_debug after > some insn, DF hasn't been run in between and thus the pseudos might be live > afterwards. Frankly moving down last_combined_insn to undobuf.other_insn in the UNDO_MODE case seems a little overengineered at this point. > If you just want to avoid a global variable, the code can be surely changed > to have a local variable from combine_instructions and pass address to that > to all try_combine calls, but other than that I think we should do what the > patch does. I'd eliminate the global variable and directly pass the insn to try_combine, this is good enough for now IMO. -- Eric Botcazou
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
On Sat, Apr 02, 2011 at 05:42:55PM +0200, Eric Botcazou wrote: > > Combiner has code to adjust this too (propagate_for_debug), but only > > updates debug insns between i2 and i3 (resp. i3 and undobuf.other_insn). > > The problem on the testcase is that this is a retry, so first > > try_combine with a later i3 calls propagate_for_debug and changes debug > > insns before that later i3, then returns an earlier insn that should be > > retried and we stop adjusting debug insns at that earlier i3. > > Unfortunately as later debug insns have been already updated earlier, they > > need to be adjusted too. > > > > The following patch fixes that by always stopping on the latest i3 that has > > been successfully combined into in the current bb, bootstrapped/regtested > > on x86_64-linux and i686-linux, ok for trunk/4.6? > > This seems to be a rare problem though so I'm not sure we should directly go > for an "always" approach. IIUC this can happen only when try_combine returns > a NEXT to combine_instructions which is before INSN in the stream, right? Yeah, but that is when in the patch i3 will differ from last_combined_insn. > Can't we precisely detect this case under the "retry:" label and pass an > additional argument to try_combine? Do we really need to adjust all the > calls We'd still need to do the LUID checking anyway, to find out when we still do retry earlier insns and when we return back to looking at new insns. Plus, I believe whenever we propagate_for_debug after i3 (the updating in between i3 and undobuf.other_insn, we need to adjust those insns too. > to propagate_for_debug and not just the ones made for UNDO_MODE when the > register is I2DEST? I think we need to update there in all cases. The reason we don't need to update beyond i3 resp. undobuf.other_insn is that DF guarantees us that there won't be debug insns referring to those pseudos afterwards, otherwise either the pseudo must be live afterwards in real code (then it wouldn't be a single use case), or debug insns would be reset, or a debug temporary would be created, where the temporary is set before last place where the pseudo is used in real code. Now, once we propagate_for_debug after some insn, DF hasn't been run in between and thus the pseudos might be live afterwards. If you just want to avoid a global variable, the code can be surely changed to have a local variable from combine_instructions and pass address to that to all try_combine calls, but other than that I think we should do what the patch does. Alex, do you agree? Jakub
Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)
> Combiner has code to adjust this too (propagate_for_debug), but only > updates debug insns between i2 and i3 (resp. i3 and undobuf.other_insn). > The problem on the testcase is that this is a retry, so first > try_combine with a later i3 calls propagate_for_debug and changes debug > insns before that later i3, then returns an earlier insn that should be > retried and we stop adjusting debug insns at that earlier i3. > Unfortunately as later debug insns have been already updated earlier, they > need to be adjusted too. > > The following patch fixes that by always stopping on the latest i3 that has > been successfully combined into in the current bb, bootstrapped/regtested > on x86_64-linux and i686-linux, ok for trunk/4.6? This seems to be a rare problem though so I'm not sure we should directly go for an "always" approach. IIUC this can happen only when try_combine returns a NEXT to combine_instructions which is before INSN in the stream, right? Can't we precisely detect this case under the "retry:" label and pass an additional argument to try_combine? Do we really need to adjust all the calls to propagate_for_debug and not just the ones made for UNDO_MODE when the register is I2DEST? -- Eric Botcazou
[PATCH] Fix VTA updating in the combiner (PR debug/48343)
Hi!
We ICE on the attached testcase, because combiner changes mode of
a pseudo (which was set just once and used once plus in debug insns)
from SImode to QImode, but the uses in debug insns aren't adjusted.
Combiner has code to adjust this too (propagate_for_debug), but only updates
debug insns between i2 and i3 (resp. i3 and undobuf.other_insn).
The problem on the testcase is that this is a retry, so first
try_combine with a later i3 calls propagate_for_debug and changes debug
insns before that later i3, then returns an earlier insn that should be
retried and we stop adjusting debug insns at that earlier i3. Unfortunately
as later debug insns have been already updated earlier, they need to be
adjusted too.
The following patch fixes that by always stopping on the latest i3 that has
been successfully combined into in the current bb, bootstrapped/regtested
on x86_64-linux and i686-linux, ok for trunk/4.6?
2011-03-30 Jakub Jelinek
PR debug/48343
* combine.c (last_combined_insn): New variable.
(combine_instructions): Clear it before processing each bb
and after last bb.
(try_combine): Set it to i3 if i3 is after it.
Use it as the last argument to propagate_for_debug instead
of i3.
(distribute_notes): Assert SET_INSN_DELETE is not called
on last_combined_insn.
* gcc.dg/torture/pr48343.c: New test.
--- gcc/combine.c.jj2011-03-23 09:34:40.0 +0100
+++ gcc/combine.c 2011-03-30 17:39:09.0 +0200
@@ -337,6 +337,9 @@ static enum machine_mode nonzero_bits_mo
static int nonzero_sign_valid;
+/* Last insn on which try_combine has been called in the current BB. */
+
+static rtx last_combined_insn;
/* Record one modification to rtl structure
to be undone by storing old_contents into *where. */
@@ -1168,6 +1171,7 @@ combine_instructions (rtx f, unsigned in
|| single_pred (this_basic_block) != last_bb)
label_tick_ebb_start = label_tick;
last_bb = this_basic_block;
+ last_combined_insn = NULL_RTX;
rtl_profile_for_bb (this_basic_block);
for (insn = BB_HEAD (this_basic_block);
@@ -1379,6 +1383,7 @@ combine_instructions (rtx f, unsigned in
}
}
+ last_combined_insn = NULL_RTX;
default_rtl_profile ();
clear_log_links ();
clear_bb_flags ();
@@ -3830,6 +3835,10 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
return 0;
}
+ if (last_combined_insn == NULL_RTX
+ || DF_INSN_LUID (last_combined_insn) < DF_INSN_LUID (i3))
+last_combined_insn = i3;
+
if (MAY_HAVE_DEBUG_INSNS)
{
struct undo *undo;
@@ -3851,7 +3860,7 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
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, i3, reg, i2src);
+ propagate_for_debug (i2, last_combined_insn, reg, i2src);
i2scratch = false;
/* Put back the new mode. */
adjust_reg_mode (reg, new_mode);
@@ -3859,18 +3868,17 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
else
{
rtx tempreg = gen_raw_REG (old_mode, REGNO (reg));
- rtx first, last;
+ rtx first;
if (reg == i2dest)
- {
- first = i2;
- last = i3;
- }
+ first = i2;
else
{
first = i3;
- last = undobuf.other_insn;
- gcc_assert (last);
+ gcc_assert (undobuf.other_insn);
+ if (DF_INSN_LUID (undobuf.other_insn)
+ > DF_INSN_LUID (last_combined_insn))
+ last_combined_insn = undobuf.other_insn;
}
/* We're dealing with a reg that changed mode but not
@@ -3882,9 +3890,9 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
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);
+ propagate_for_debug (first, last_combined_insn, reg, tempreg);
adjust_reg_mode (reg, new_mode);
- propagate_for_debug (first, last, tempreg,
+ propagate_for_debug (first, last_combined_insn, tempreg,
lowpart_subreg (old_mode, reg, new_mode));
}
}
@@ -4099,14 +4107,14 @@ try_combine (rtx i3, rtx i2, rtx i1, rtx
if (newi2pat)
{
if (MAY_HAVE_DEBUG_INSNS && i2scratch)
- propagate_for_debug (i2, i3, i2dest, i2src);
+ propagate_for_debug (i2, last_combined_insn, i2dest, i2src);
INSN_CODE
