Re: [PATCH] Fix VTA updating in the combiner (PR debug/48343)

[Jakub Jelinek <jakub at redhat dot com>]

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