Patchwork fix latent compare-debug problem in cprop

login
register
mail settings
Submitter Alexandre Oliva
Date June 4, 2011, 12:50 p.m.
Message ID <orboydg3q9.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/98725/
State New
Headers show

Comments

Alexandre Oliva - June 4, 2011, 12:50 p.m.
If cprop regards changes to debug insns as “changed”, it will perform
cfg optimizations and more, even if no non-debug insns were changed,
causing divergence between -g and -g0 compilations.

This was observed during bootstrap-debug-lib of the SSA coalesce patch,
building a-strsea.adb with -fcompare-debug.  Because of different CFGs,
cse2 chose different equivalent pseudos for an insn.  -fcompare-debug
detected the difference in REG notes, even though the executable code
turned out to be the same.  We can't count on this luck, though: other
optimization passes could apply different transformations to the
different CFGs, producing different executable code, which must never
happen.

This patch fixes the problem, disregarding changes to debug insns as
“changed”.  Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to
install?
Steven Bosscher - June 4, 2011, 2:05 p.m.
On Sat, Jun 4, 2011 at 2:50 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> If cprop regards changes to debug insns as “changed”, it will perform
> cfg optimizations and more, even if no non-debug insns were changed,
> causing divergence between -g and -g0 compilations.
>
> This was observed during bootstrap-debug-lib of the SSA coalesce patch,
> building a-strsea.adb with -fcompare-debug.  Because of different CFGs,
> cse2 chose different equivalent pseudos for an insn.  -fcompare-debug
> detected the difference in REG notes, even though the executable code
> turned out to be the same.  We can't count on this luck, though: other
> optimization passes could apply different transformations to the
> different CFGs, producing different executable code, which must never
> happen.
>
> This patch fixes the problem, disregarding changes to debug insns as
> “changed”.  Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to
> install?

Looks OK to me, although I can't approve it.

I'm curious, though: What CFG changes or other transformations are
performed without this patch? It could be a sign of a missed
optimization before CPROP. Have you looked at that too?

Ciao!
Steven
Eric Botcazou - June 5, 2011, 10:51 p.m.
> This patch fixes the problem, disregarding changes to debug insns as
> “changed”.  Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to
> install?

Yes, thanks.
Alexandre Oliva - June 6, 2011, 6:33 a.m.
On Jun  4, 2011, Steven Bosscher <stevenb.gcc@gmail.com> wrote:

> I'm curious, though: What CFG changes or other transformations are
> performed without this patch? It could be a sign of a missed
> optimization before CPROP. Have you looked at that too?

IIRC the transformation that was possible but that hadn't been performed
yet involved merging two blocks (I remember block numbering was off
after cprop, and that there were cfgoptimize notes in its -g dumps, but
not in the non-g dumps), but I didn't look any further than that.

Patch

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* cprop.c (local_cprop_pass): Don't set changed for debug insns.

Index: gcc/cprop.c
===================================================================
--- gcc/cprop.c.orig	2011-06-04 05:09:24.414816329 -0300
+++ gcc/cprop.c	2011-06-04 05:09:25.954797626 -0300
@@ -1223,7 +1223,8 @@  local_cprop_pass (void)
 		    {
 		      if (do_local_cprop (reg_use_table[i], insn))
 			{
-			  changed = true;
+			  if (!DEBUG_INSN_P (insn))
+			    changed = true;
 			  break;
 			}
 		    }