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  <jakub@redhat.com>

	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.


	Jakub

> 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?

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

> 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.