fixed two issues in handling debug_insn.

Submitter	Bingfeng Mei
Date	July 16, 2010, 12:27 p.m.
Message ID	<7FB04A5C213E9943A72EE127DB74F0ADA6897ACFFB@SJEXCHCCR02.corp.ad.broadcom.com>
Download	mbox \| patch
Permalink	/patch/59099/
State	New
Headers	show Return-Path: <gcc-patches-return-268352-incoming=patchwork.ozlabs.org@gcc.gnu.org> From: "Bingfeng Mei" <bmei@broadcom.com> To: "Jakub Jelinek" <jakub@redhat.com> cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org> Date: Fri, 16 Jul 2010 05:27:40 -0700 Subject: RE: [PATCH] fixed two issues in handling debug_insn. Message-ID: <7FB04A5C213E9943A72EE127DB74F0ADA6897ACFFB@SJEXCHCCR02.corp.ad.broadcom.com> References: <7FB04A5C213E9943A72EE127DB74F0ADA6897ACFF2@SJEXCHCCR02.corp.ad.broadcom.com> <20100716122420.GT20208@tyan-ft48-01.lab.bos.redhat.com> In-Reply-To: <20100716122420.GT20208@tyan-ft48-01.lab.bos.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk Sender: gcc-patches-owner@gcc.gnu.org

Comments

Bingfeng Mei - July 16, 2010, 12:27 p.m.

Well, I don't know that function. Here is the updated patch. OK for
trunk & 4.5? 


> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: 16 July 2010 13:24
> To: Bingfeng Mei
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] fixed two issues in handling debug_insn.
> 
> On Fri, Jul 16, 2010 at 05:15:21AM -0700, Bingfeng Mei wrote:
> 
> > --- loop-doloop.c       (revision 162258)
> > +++ loop-doloop.c       (working copy)
> > @@ -104,11 +104,15 @@
> >    if (GET_CODE (pattern) != PARALLEL)
> >      {
> >        rtx cond;
> > +      rtx prev_insn = PREV_INSN (doloop_pat);
> >
> > -      /* We expect the decrement to immediately precede the branch.
> */
> > +      /* We expect the decrement to immediately precede the branch.
> > +         Need to skip possible debug_insn */
> > +      while (prev_insn != NULL_RTX && DEBUG_INSN_P (prev_insn))
> > +        prev_insn = PREV_INSN (prev_insn);
> >
> > -      if ((PREV_INSN (doloop_pat) == NULL_RTX)
> > -          || !INSN_P (PREV_INSN (doloop_pat)))
> > +      if ((prev_insn == NULL_RTX)
> > +          || !INSN_P (prev_insn))
> >          return 0;
> >
> >        cmp = pattern;
> 
> Why don't you just do
>       rtx prev_insn = prev_nondebug_insn (doloop_pat);
> 
>       if (prev_insn == NULL_RTX || !INSN_P (prev_insn))
> 	return 0;
> 
> ?
> 

> 	Jakub

Jakub Jelinek - July 16, 2010, 12:37 p.m.

On Fri, Jul 16, 2010 at 05:27:40AM -0700, Bingfeng Mei wrote:
> Well, I don't know that function. Here is the updated patch. OK for
> trunk & 4.5? 

> +      rtx prev_insn = prev_nondebug_insn (doloop_pat);
> 
>        /* We expect the decrement to immediately precede the branch.  */
> 
>        if ((PREV_INSN (doloop_pat) == NULL_RTX)
> -          || !INSN_P (PREV_INSN (doloop_pat)))
> +          || !INSN_P (prev_insn))
>          return 0;

This will crash if PREV_INSN is non-NULL, but prev_nondebug_insn returns
NULL.  And, please remove the redundant parens and unnecessary splitting
of the condition on two lines, i.e. what I wrote:

       if (prev_insn == NULL_RTX || !INSN_P (prev_insn))

It looks good to me with those changes (also the ddg.c change), but I can't
approve this.

	Jakub

Patch

Index: ddg.c
===================================================================
--- ddg.c       (revision 162258)
+++ ddg.c       (working copy)
@@ -488,7 +488,7 @@ 
     }

   /* There is nothing to do for this BB.  */
-  if (num_nodes <= 1)
+  if ((num_nodes - g->num_debug) <= 1)
     {
       free (g);
       return NULL;
Index: loop-doloop.c
===================================================================
--- loop-doloop.c       (revision 162258)
+++ loop-doloop.c       (working copy)
@@ -104,11 +104,12 @@ 
   if (GET_CODE (pattern) != PARALLEL)
     {
       rtx cond;
+      rtx prev_insn = prev_nondebug_insn (doloop_pat);

       /* We expect the decrement to immediately precede the branch.  */

       if ((PREV_INSN (doloop_pat) == NULL_RTX)
-          || !INSN_P (PREV_INSN (doloop_pat)))
+          || !INSN_P (prev_insn))
         return 0;

       cmp = pattern;

Patchwork fixed two issues in handling debug_insn.

Comments

Patch