debug insns in SMS (was: Re: Debug_insn)

debug insns in SMS (was: Re: Debug_insn)

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

On May  3, 2011, Revital1 Eres <ERES@il.ibm.com> wrote:

> Please let me know if you need any further info.

No, thanks, that was all I needed.

I think this will restore proper functioning to SMS in the presence of
debug insns.  A while ago, we'd never generate deps of non-debug insns
on debug insns.  I introduced them to enable sched to adjust (reset)
debug insns when non-debug insns were moved before them.  I believe it
is safe to leave them out of the SCCs.  Even though this will end up
causing some loss of debug info, that's probably unavoidable, and the
end result after this change is pobably the best we can hope for.  Your
thoughts?

Is this ok to install if it regstraps successfully?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ddg-debug-deps.patch --]
[-- Type: text/x-diff, Size: 722 bytes --]

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

	* ddg.c (build_intra_loop_deps): Discard deps of nondebug on debug.

Index: gcc/ddg.c
===================================================================
--- gcc/ddg.c.orig	2011-05-04 03:42:20.938013725 -0300
+++ gcc/ddg.c	2011-05-04 03:42:30.202253457 -0300
@@ -452,7 +452,12 @@ build_intra_loop_deps (ddg_ptr g)
 
       FOR_EACH_DEP (dest_node->insn, SD_LIST_BACK, sd_it, dep)
 	{
-	  ddg_node_ptr src_node = get_node_of_insn (g, DEP_PRO (dep));
+	  ddg_node_ptr src_node;
+
+	  if (DEBUG_INSN_P (DEP_PRO (dep)) && !DEBUG_INSN_P (dest_node->insn))
+	    continue;
+
+	  src_node = get_node_of_insn (g, DEP_PRO (dep));
 
 	  if (!src_node)
 	    continue;

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://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: debug insns in SMS

[Alexandre Oliva]

On May  4, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:

> I think this will restore proper functioning to SMS in the presence of
> debug insns.  A while ago, we'd never generate deps of non-debug insns
> on debug insns.  I introduced them to enable sched to adjust (reset)
> debug insns when non-debug insns were moved before them.  I believe it
> is safe to leave them out of the SCCs.  Even though this will end up
> causing some loss of debug info, that's probably unavoidable, and the
> end result after this change is pobably the best we can hope for.  Your
> thoughts?

Actually...  We can probably do better.  Leaving the debug insns out, we
have no way to adjust (reset) them when non-debug insns that depended on
them were moved across them, rendering the debug value incorrect.

I'm now trying to find out some way to add the tracking of these deps to
the DDG in a way that doesn't change the SCCs because of -g, but that
enables us to adjust (or even modulo-schedule) debug insns accurately.

Suggestions would be appreciated.

-- 
Alexandre Oliva, freedom fighter    http://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: debug insns in SMS (was: Re: Debug_insn)

[Revital1 Eres]

Hello Alexandre

> I think this will restore proper functioning to SMS in the presence of
> debug insns.  A while ago, we'd never generate deps of non-debug insns
> on debug insns.  I introduced them to enable sched to adjust (reset)
> debug insns when non-debug insns were moved before them.  I believe it
> is safe to leave them out of the SCCs.  Even though this will end up
> causing some loss of debug info, that's probably unavoidable, and the
> end result after this change is pobably the best we can hope for.  Your
> thoughts?

Thanks for the patch!

I actually discussed this issue with Ayal yesterday.
Ayal also suggested to reconsider the edges that are created in
the DDG between real instructions and debug_insns. Currently, we create
bidirectional anti deps edges between them. This leads to the problem you
were trying to solve in the current patch (described below) where these
extra edges influence the construction of the strongly connected component
and the code generated with and w\o -g. Your patch seems to solve this
problem.
However I can not approve it as I'm not the maintainer (Ayal is).

Another problem with the handling of debug insns in SMS is that
debug_insns instructions are been ignored while scheduling which means
that they do not delimit the scheduling window of the real instructions
they depend on. This could lead to a scenario where the dependencies
between real instruction and debug_insn could be violated as we leave
the debug_insns in their original place after scheduling. I'll try to send
you
an example of this problem as well.

Example of code gen difference when using -g and without it which this
patch tries to solve:
The following nodes are all belong to the same SCC running with -g.
node 1 is not present in this SCC when not using -g.
(the nodes marked in [] are the one that do not
exist with your patch and prevent from node 1 to be added to the SCC
when compiling with -g)

                                                                     node 0
Debug_insn  (ior(MEM(ivtmp.24),MEM(ivtmp.46))
in edges: 1->0, 2->0
out edges: [0->1], [0->4], [0->2]


node 1
Reg 220 = MEM (pre_inc (ivtmp.24))
in edges: [0->1], 1->1
out edges: 1->0, 1->1, 1->3

node 2
Reg 221= MEM (pre_inc (ivtmp.46))
in edges: [0->2], 4->2, 2->2
out edges: 2->0, 2->3, 2->4, 2->2

node 3
Reg 222= ior (220,221)
in edges: 2->3, 1->3
out edges: 3->4

node 4
MEM[pre_inc(196)] = 222
in edges: 3->4, 4->4, [0->4], 2->4
out edges: 4->4, 4->2

Thanks,
Revital

Re: debug insns in SMS

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]

On May  4, 2011, Revital1 Eres <ERES@il.ibm.com> wrote:

> Hello Alexandre
>> I think this will restore proper functioning to SMS in the presence of
>> debug insns.  A while ago, we'd never generate deps of non-debug insns
>> on debug insns.  I introduced them to enable sched to adjust (reset)
>> debug insns when non-debug insns were moved before them.  I believe it
>> is safe to leave them out of the SCCs.  Even though this will end up
>> causing some loss of debug info, that's probably unavoidable, and the
>> end result after this change is pobably the best we can hope for.  Your
>> thoughts?

> Thanks for the patch!

> I actually discussed this issue with Ayal yesterday.
> Ayal also suggested to reconsider the edges that are created in
> the DDG between real instructions and debug_insns. Currently, we create
> bidirectional anti deps edges between them. This leads to the problem you
> were trying to solve in the current patch (described below) where these
> extra edges influence the construction of the strongly connected component
> and the code generated with and w\o -g. Your patch seems to solve this
> problem.
> However I can not approve it as I'm not the maintainer (Ayal is).

Ping?

(Retested on x86_64-linux-gnu and i686-pc-linux-gnu)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ddg-debug-deps.patch --]
[-- Type: text/x-diff, Size: 722 bytes --]

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

	* ddg.c (build_intra_loop_deps): Discard deps of nondebug on debug.

Index: gcc/ddg.c
===================================================================
--- gcc/ddg.c.orig	2012-01-04 21:06:38.000000000 -0200
+++ gcc/ddg.c	2012-04-08 02:10:44.711511989 -0300
@@ -532,7 +532,12 @@ build_intra_loop_deps (ddg_ptr g)
 
       FOR_EACH_DEP (dest_node->insn, SD_LIST_BACK, sd_it, dep)
 	{
-	  ddg_node_ptr src_node = get_node_of_insn (g, DEP_PRO (dep));
+	  ddg_node_ptr src_node;
+
+	  if (DEBUG_INSN_P (DEP_PRO (dep)) && !DEBUG_INSN_P (dest_node->insn))
+	    continue;
+
+	  src_node = get_node_of_insn (g, DEP_PRO (dep));
 
 	  if (!src_node)
 	    continue;

[-- Attachment #3: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva, freedom fighter    http://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: debug insns in SMS

[Alexandre Oliva]

On Apr  9, 2012, Alexandre Oliva <aoliva@redhat.com> wrote:

>>> I think this will restore proper functioning to SMS in the presence of
>>> debug insns.  A while ago, we'd never generate deps of non-debug insns
>>> on debug insns.  I introduced them to enable sched to adjust (reset)
>>> debug insns when non-debug insns were moved before them.  I believe it
>>> is safe to leave them out of the SCCs.  Even though this will end up
>>> causing some loss of debug info, that's probably unavoidable, and the
>>> end result after this change is pobably the best we can hope for.  Your
>>> thoughts?

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

> 	* ddg.c (build_intra_loop_deps): Discard deps of nondebug on debug.

> Ping?

http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00419.html

-- 
Alexandre Oliva, freedom fighter    http://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: debug insns in SMS

[Alexandre Oliva]

Apologies for the duplicate ping, this one is now properly addressed to
the pass maintainer.

On Apr  9, 2012, Alexandre Oliva <aoliva@redhat.com> wrote:

> On May  4, 2011, Revital1 Eres <ERES@il.ibm.com> wrote:
>> Hello Alexandre
>>> I think this will restore proper functioning to SMS in the presence of
>>> debug insns.  A while ago, we'd never generate deps of non-debug insns
>>> on debug insns.  I introduced them to enable sched to adjust (reset)
>>> debug insns when non-debug insns were moved before them.  I believe it
>>> is safe to leave them out of the SCCs.  Even though this will end up
>>> causing some loss of debug info, that's probably unavoidable, and the
>>> end result after this change is pobably the best we can hope for.  Your
>>> thoughts?

>> Thanks for the patch!

>> I actually discussed this issue with Ayal yesterday.
>> Ayal also suggested to reconsider the edges that are created in
>> the DDG between real instructions and debug_insns. Currently, we create
>> bidirectional anti deps edges between them. This leads to the problem you
>> were trying to solve in the current patch (described below) where these
>> extra edges influence the construction of the strongly connected component
>> and the code generated with and w\o -g. Your patch seems to solve this
>> problem.
>> However I can not approve it as I'm not the maintainer (Ayal is).

> Ping?

> (Retested on x86_64-linux-gnu and i686-pc-linux-gnu)


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

> 	* ddg.c (build_intra_loop_deps): Discard deps of nondebug on debug.

Ping?  http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00419.html

-- 
Alexandre Oliva, freedom fighter    http://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: debug insns in SMS

[Ayal Zaks]

Thanks for the duplicate ping. This is fine.
So this indeed solves the discrepancy between running SMS w/ and w/o debugging?
Please include a comment next to the code stating why it's important
not to create such deps.
You may also want to store the result of "DEP_PRO (dep)" in
src_<something> and use it twice, for clarity.
Thanks,
Ayal.


On Thu, Jun 14, 2012 at 1:00 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
>
> Apologies for the duplicate ping, this one is now properly addressed to
> the pass maintainer.
>
> On Apr  9, 2012, Alexandre Oliva <aoliva@redhat.com> wrote:
>
> > On May  4, 2011, Revital1 Eres <ERES@il.ibm.com> wrote:
> >> Hello Alexandre
> >>> I think this will restore proper functioning to SMS in the presence of
> >>> debug insns.  A while ago, we'd never generate deps of non-debug insns
> >>> on debug insns.  I introduced them to enable sched to adjust (reset)
> >>> debug insns when non-debug insns were moved before them.  I believe it
> >>> is safe to leave them out of the SCCs.  Even though this will end up
> >>> causing some loss of debug info, that's probably unavoidable, and the
> >>> end result after this change is pobably the best we can hope for.
> >>>  Your
> >>> thoughts?
>
> >> Thanks for the patch!
>
> >> I actually discussed this issue with Ayal yesterday.
> >> Ayal also suggested to reconsider the edges that are created in
> >> the DDG between real instructions and debug_insns. Currently, we create
> >> bidirectional anti deps edges between them. This leads to the problem
> >> you
> >> were trying to solve in the current patch (described below) where these
> >> extra edges influence the construction of the strongly connected
> >> component
> >> and the code generated with and w\o -g. Your patch seems to solve this
> >> problem.
> >> However I can not approve it as I'm not the maintainer (Ayal is).
>
> > Ping?
>
> > (Retested on x86_64-linux-gnu and i686-pc-linux-gnu)
>
>
> > for  gcc/ChangeLog
> > from  Alexandre Oliva  <aoliva@redhat.com>
>
> >       * ddg.c (build_intra_loop_deps): Discard deps of nondebug on
> > debug.
>
> Ping?  http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00419.html
>
> --
> Alexandre Oliva, freedom fighter    http://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: debug insns in SMS

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Jun 14, 2012, Ayal Zaks <ayal.zaks@gmail.com> wrote:

> Thanks for the duplicate ping. This is fine.

> So this indeed solves the discrepancy between running SMS w/ and w/o
> debugging?

I can't tell whether it solves it completely, but it's surely a step in
the right direction.

> Please include a comment next to the code stating why it's important
> not to create such deps.

Done

> You may also want to store the result of "DEP_PRO (dep)" in
> src_<something> and use it twice, for clarity.

Done

Here's what I'm about to check in.  Thanks,


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ddg-debug-deps.patch --]
[-- Type: text/x-diff, Size: 870 bytes --]

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

	* ddg.c (build_intra_loop_deps): Discard deps of nondebug on debug.

Index: gcc/ddg.c
===================================================================
--- gcc/ddg.c.orig	2012-06-21 21:45:00.305313798 -0300
+++ gcc/ddg.c	2012-06-21 21:51:58.000000000 -0300
@@ -531,7 +531,15 @@ build_intra_loop_deps (ddg_ptr g)
 
       FOR_EACH_DEP (dest_node->insn, SD_LIST_BACK, sd_it, dep)
 	{
-	  ddg_node_ptr src_node = get_node_of_insn (g, DEP_PRO (dep));
+	  rtx src_insn = DEP_PRO (dep);
+	  ddg_node_ptr src_node;
+
+	  /* Don't add dependencies on debug insns to non-debug insns
+	     to avoid codegen differences between -g and -g0.  */
+	  if (DEBUG_INSN_P (src_insn) && !DEBUG_INSN_P (dest_node->insn))
+	    continue;
+
+	  src_node = get_node_of_insn (g, src_insn);
 
 	  if (!src_node)
 	    continue;

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://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