[Ping] [PATCH, ifcvt] Fix PR63917

["Zhenqiang Chen" <zhenqiang dot chen at arm dot com>]

Ping?

Thanks!
-Zhenqiang

-----Original Message-----
From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of Zhenqiang Chen
Sent: Monday, December 01, 2014 3:14 PM
To: 'H.J. Lu'
Cc: Richard Henderson; GCC Patches
Subject: RE: [PATCH, ifcvt] Fix PR63917


> -----Original Message-----
> From: H.J. Lu [mailto:hjl.tools@gmail.com]
> Sent: Friday, November 28, 2014 10:45 PM
> To: Zhenqiang Chen
> Cc: Richard Henderson; GCC Patches
> Subject: Re: [PATCH, ifcvt] Fix PR63917
> 
> On Sun, Nov 23, 2014 at 7:47 PM, Zhenqiang Chen 
> <zhenqiang.chen@arm.com> wrote:
> >
> >> -----Original Message-----
> >> From: Richard Henderson [mailto:rth@redhat.com]
> >> Sent: Friday, November 21, 2014 2:27 AM
> >> To: Zhenqiang Chen; gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH, ifcvt] Fix PR63917
> >>
> >> On 11/20/2014 10:48 AM, Zhenqiang Chen wrote:
> >> > +/* Check X clobber CC reg or not.  */
> >> > +
> >> > +static bool
> >> > +clobber_cc_p (rtx x)
> >> > +{
> >> > +  RTX_CODE code = GET_CODE (x);
> >> > +  int i;
> >> > +
> >> > +  if (code == CLOBBER
> >> > +      && REG_P (XEXP (x, 0))
> >> > +      && (GET_MODE_CLASS (GET_MODE (XEXP (x, 0))) == MODE_CC))
> >> > +    return TRUE;
> >> > +  else if (code == PARALLEL)
> >> > +    for (i = 0; i < XVECLEN (x, 0); i++)
> >> > +      if (clobber_cc_p (XVECEXP (x, 0, i)))
> >> > +   return TRUE;
> >> > +  return FALSE;
> >> > +}
> >>
> >> Why would you need something like this when modified_between_p or
> one
> >> of its kin ought to do the job?
> >
> > Thanks for the comments. Patch is updated to use set_of.
> >
> > And it is also enhanced to make sure that the new generated insns 
> > can not clobber CC.
> >
> > Bootstrap and no make check regression on X86-64 and IA32.
> >
> > OK for trunk?
> >
> > Thanks!
> > -Zhenqiang
> >
> > ChangeLog:
> > 2014-11-24  Zhenqiang Chen  <zhenqiang.chen@arm.com>
> >
> >         PR rtl-optimization/63917
> >         * ifcvt.c (cc_in_cond): New function.
> >         (end_ifcvt_sequence): Make sure new generated insns do not 
> > clobber CC.
> >         (noce_process_if_block, check_cond_move_block): Check CC
> references.
> >
> > testsuite/ChangeLog:
> > 2014-11-24  Zhenqiang Chen  <zhenqiang.chen@arm.com>
> >
> >         * gcc.dg/pr63917.c: New test.
> >
> > diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 21f08c2..1acd0ff 100644
> > --- a/gcc/ifcvt.c
> > +++ b/gcc/ifcvt.c
> > @@ -1016,6 +1016,18 @@ noce_emit_move_insn (rtx x, rtx y)
> >                    0, 0, outmode, y);  }
> >
> > +/* Return the CC reg if it is used in COND.  */
> > +
> > +static rtx
> > +cc_in_cond (rtx cond)
> > +{
> > +  if ((HAVE_cbranchcc4) && cond
> > +      && (GET_MODE_CLASS (GET_MODE (XEXP (cond, 0))) == MODE_CC))
> > +    return XEXP (cond, 0);
> > +
> > +  return NULL_RTX;
> > +}
> > +
> >  /* Return sequence of instructions generated by if conversion.  This
> >     function calls end_sequence() to end the current stream, ensures
> >     that are instructions are unshared, recognizable non-jump insns.
> > @@ -1026,6 +1038,7 @@ end_ifcvt_sequence (struct noce_if_info
> > *if_info)  {
> >    rtx_insn *insn;
> >    rtx_insn *seq = get_insns ();
> > +  rtx cc = cc_in_cond (if_info->cond);
> >
> >    set_used_flags (if_info->x);
> >    set_used_flags (if_info->cond);
> > @@ -1040,7 +1053,9 @@ end_ifcvt_sequence (struct noce_if_info *if_info)
> >       allows proper placement of required clobbers.  */
> >    for (insn = seq; insn; insn = NEXT_INSN (insn))
> >      if (JUMP_P (insn)
> > -       || recog_memoized (insn) == -1)
> > +       || recog_memoized (insn) == -1
> > +          /* Make sure new generated code does not clobber CC.  */
> > +       || (cc && set_of (cc, insn)))
> >        return NULL;
> >
> >    return seq;
> > @@ -2544,6 +2559,7 @@ noce_process_if_block (struct noce_if_info
> *if_info)
> >    rtx_insn *insn_a, *insn_b;
> >    rtx set_a, set_b;
> >    rtx orig_x, x, a, b;
> > +  rtx cc;
> >
> >    /* We're looking for patterns of the form
> >
> > @@ -2655,6 +2671,13 @@ noce_process_if_block (struct noce_if_info
> *if_info)
> >    if_info->a = a;
> >    if_info->b = b;
> >
> > +  /* Skip it if the instruction to be moved might clobber CC.  */  
> > + cc = cc_in_cond (cond);  if (cc)
> > +    if (set_of (cc, insn_a)
> > +       || (insn_b && set_of (XEXP (cond, 0), insn_b)))
> > +      return FALSE;
> > +
> >    /* Try optimizations in some approximation of a useful order.  */
> >    /* ??? Should first look to see if X is live incoming at all.  If it
> >       isn't, we don't need anything but an unconditional set.  */ @@
> > -2811,6 +2834,7 @@ check_cond_move_block (basic_block bb,
> >                        rtx cond)
> >  {
> >    rtx_insn *insn;
> > +  rtx cc = cc_in_cond (cond);
> >
> >     /* We can only handle simple jumps at the end of the basic block.
> >        It is almost impossible to update the CFG otherwise.  */ @@
> > -2868,6 +2892,10 @@ check_cond_move_block (basic_block bb,
> >           && modified_between_p (src, insn, NEXT_INSN (BB_END (bb))))
> >         return FALSE;
> >
> > +      /* Skip it if the instruction to be moved might clobber CC.  */
> > +      if (cc && set_of (cc, insn))
> > +       return FALSE;
> > +
> >        vals->put (dest, src);
> >
> >        regs->safe_push (dest);
> > diff --git a/gcc/testsuite/gcc.dg/pr63917.c 
> > b/gcc/testsuite/gcc.dg/pr63917.c new file mode 100644 index 
> > 0000000..422b15d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr63917.c
> 
> It should be pr64007.c.
> 
> > @@ -0,0 +1,45 @@
> > +/* { dg-options " -O3 " } */
> 
> You need
> 
> /* { dg-do run } */
> 
> since it is a run-time test.
> 
> > +
> > +int d, i;
> > +
> > +struct S
> > +{
> > +  int f0;
> > +} *b, c, e, h, **g = &b;
> > +
> > +static struct S *f = &e;
> > +
> > +int
> > +fn1 (int p)
> > +{
> > +  int a = 0;
> > +  return a || p < 0 || p >= 2 || 1 >> p; }
> > +
> > +int
> > +main ()
> > +{
> > +  int k = 1, l, *m = &c.f0;
> > +
> > +  for (;;)
> > +    {
> > +      l = fn1 (i);
> > +      *m = k && i;
> > +      if (l)
> > +       {
> > +         int n[1] = {0};
> > +       }
> > +      break;
> > +    }
> > +
> > +  *g = &h;
> > +
> > +  if (d)
> > +    (*m)--;
> > +  d = (f != 0) | (i >= 0);
> > +
> > +  if (c.f0 != 0)
> > +    __builtin_abort ();
> > +
> > +  return 0;
> > +}
> >
> 
> You test doesn't fail without the fix since your test is a little bit 
> different from the test at
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64007
> 
> You missed:
> 
> assert (b);
> 
> Without it, the bug won't be triggered.

Thanks for the comments. Test case in the patch is updated.

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 21f08c2..1acd0ff 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1016,6 +1016,18 @@ noce_emit_move_insn (rtx x, rtx y)
 		   0, 0, outmode, y);
 }
 
+/* Return the CC reg if it is used in COND.  */
+
+static rtx
+cc_in_cond (rtx cond)
+{
+  if ((HAVE_cbranchcc4) && cond
+      && (GET_MODE_CLASS (GET_MODE (XEXP (cond, 0))) == MODE_CC))
+    return XEXP (cond, 0);
+
+  return NULL_RTX;
+}
+
 /* Return sequence of instructions generated by if conversion.  This
    function calls end_sequence() to end the current stream, ensures
    that are instructions are unshared, recognizable non-jump insns.
@@ -1026,6 +1038,7 @@ end_ifcvt_sequence (struct noce_if_info *if_info)  {
   rtx_insn *insn;
   rtx_insn *seq = get_insns ();
+  rtx cc = cc_in_cond (if_info->cond);
 
   set_used_flags (if_info->x);
   set_used_flags (if_info->cond);
@@ -1040,7 +1053,9 @@ end_ifcvt_sequence (struct noce_if_info *if_info)
      allows proper placement of required clobbers.  */
   for (insn = seq; insn; insn = NEXT_INSN (insn))
     if (JUMP_P (insn)
-	|| recog_memoized (insn) == -1)
+	|| recog_memoized (insn) == -1
+	   /* Make sure new generated code does not clobber CC.  */
+	|| (cc && set_of (cc, insn)))
       return NULL;
 
   return seq;
@@ -2544,6 +2559,7 @@ noce_process_if_block (struct noce_if_info *if_info)
   rtx_insn *insn_a, *insn_b;
   rtx set_a, set_b;
   rtx orig_x, x, a, b;
+  rtx cc;
 
   /* We're looking for patterns of the form
 
@@ -2655,6 +2671,13 @@ noce_process_if_block (struct noce_if_info *if_info)
   if_info->a = a;
   if_info->b = b;
 
+  /* Skip it if the instruction to be moved might clobber CC.  */
+  cc = cc_in_cond (cond);
+  if (cc)
+    if (set_of (cc, insn_a)
+	|| (insn_b && set_of (XEXP (cond, 0), insn_b)))
+      return FALSE;
+
   /* Try optimizations in some approximation of a useful order.  */
   /* ??? Should first look to see if X is live incoming at all.  If it
      isn't, we don't need anything but an unconditional set.  */ @@ -2811,6 +2834,7 @@ check_cond_move_block (basic_block bb,
 		       rtx cond)
 {
   rtx_insn *insn;
+  rtx cc = cc_in_cond (cond);
 
    /* We can only handle simple jumps at the end of the basic block.
       It is almost impossible to update the CFG otherwise.  */ @@ -2868,6 +2892,10 @@ check_cond_move_block (basic_block bb,
 	  && modified_between_p (src, insn, NEXT_INSN (BB_END (bb))))
 	return FALSE;
 
+      /* Skip it if the instruction to be moved might clobber CC.  */
+      if (cc && set_of (cc, insn))
+	return FALSE;
+
       vals->put (dest, src);
 
       regs->safe_push (dest);
diff --git a/gcc/testsuite/gcc.dg/pr64007.c b/gcc/testsuite/gcc.dg/pr64007.c new file mode 100644 index 0000000..cb0e50f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr64007.c
@@ -0,0 +1,50 @@
+/* { dg-options " -O3 " } */
+/* { dg-do run } */
+
+#include <assert.h>
+
+int d, i;
+
+struct S
+{
+  int f0;
+} *b, c, e, h, **g = &b;
+
+static struct S *f = &e;
+
+int
+fn1 (int p)
+{
+  int a = 0;
+  return a || p < 0 || p >= 2 || 1 >> p; }
+
+int
+main ()
+{
+  int k = 1, l, *m = &c.f0;
+
+  for (;;)
+    {
+      l = fn1 (i);
+      *m = k && i;
+      if (l)
+	{
+	  int n[1] = {0};
+	}
+      break;
+    }
+
+  *g = &h;
+
+  assert (b);
+
+  if (d)
+    (*m)--;
+  d = (f != 0) | (i >= 0);
+
+  if (c.f0 != 0)
+    __builtin_abort ();
+
+  return 0;
+}