RFA: fix PR66726 regression for min/max/conversion combo from PR71016 patch

Message ID 5D19EA7E.5060802@riscy-ip.com
State New
Headers show
Series
  • RFA: fix PR66726 regression for min/max/conversion combo from PR71016 patch
Related show

Commit Message

Joern Wolfgang Rennecke July 1, 2019, 11:11 a.m.
The heuristic introduced for PR71016 prevents recognizing a max / min 
combo like it is used for
saturation when followed by a conversion.
The attached patch refines the heuristic to allow this case. Regression 
tested on x86_64-pc-linux-gnu .

Comments

Richard Biener July 1, 2019, 11:38 a.m. | #1
On Mon, Jul 1, 2019 at 1:22 PM Joern Wolfgang Rennecke
<joern.rennecke@riscy-ip.com> wrote:
>

> The heuristic introduced for PR71016 prevents recognizing a max / min

> combo like it is used for

> saturation when followed by a conversion.

> The attached patch refines the heuristic to allow this case. Regression

> tested on x86_64-pc-linux-gnu .


Few style nits:

                  if (!gsi_end_p (gsi))
-                   return NULL;
+                   {
+                     gimple *assign = gsi_stmt (gsi);
+                     if (gimple_code (assign) != GIMPLE_ASSIGN)
+                       return NULL;
                       if (gassign *assign = dyn_cast <gassign *>
(gsi_stmt (gsi)))
                         {
+                     tree lhs = gimple_assign_lhs (assign);
+                     enum tree_code ass_code = gimple_assign_rhs_code (assign);
+                     if (ass_code != MAX_EXPR && ass_code != MIN_EXPR)
+                       return NULL;
+                     gsi_prev_nondebug (&gsi);
+                     if (!gsi_end_p (gsi))
+                       return NULL;
                        }
                      else
                        return NULL;
+                   }

also please check that 'lhs' is equal to gimple_assing_rhs1 (arg0_def_stmt)
otherwise you'd also allow MIN/MAX unrelated to the conversion detected.

On x86 I see the MIN_EXPR is already detected by GENERIC folding,
I wonder if that is required or if we can handle the case without in one
phiopt pass invocation as well.

Otherwise looks good to me.
Thanks,
Richard.
Joern Wolfgang Rennecke July 1, 2019, 2:05 p.m. | #2
[Apologies if this is a duplicate, I'm unsure if my previous mail was 
delivered]
On 01/07/19 12:38, Richard Biener wrote:
> On Mon, Jul 1, 2019 at 1:22 PM Joern Wolfgang Rennecke

> <joern.rennecke@riscy-ip.com> wrote:

>> The heuristic introduced for PR71016 prevents recognizing a max / min

>> combo like it is used for

>> saturation when followed by a conversion.

>> The attached patch refines the heuristic to allow this case. Regression

>> tested on x86_64-pc-linux-gnu .

> Few style nits:

>

>    ...

>

> also please check that 'lhs' is equal to gimple_assing_rhs1 (arg0_def_stmt)

> otherwise you'd also allow MIN/MAX unrelated to the conversion detected.


Like the attached patch?
>

> On x86 I see the MIN_EXPR is already detected by GENERIC folding,

> I wonder if that is required or if we can handle the case without in one

> phiopt pass invocation as well.

>

tree_ssa_phiopt_worker is supposed to handle this by handling nested 
COND_EXPRs
from the inside out:

    /* Search every basic block for COND_EXPR we may be able to optimize.

      We walk the blocks in order that guarantees that a block with
      a single predecessor is processed before the predecessor.
      This ensures that we collapse inner ifs before visiting the
      outer ones, and also that we do not try to visit a removed
      block.  */
   bb_order = single_pred_before_succ_order ();

However, I have no idea how to construct a testcase for this with the 
gimple folding in place.

#define SAT(x) (x < 0 ? 0 : (x > 255 ? 255 : x))

void
foo (unsigned char *p, int i, int m)
{
   *p = (m > SAT (i) ? m : SAT (i));
}

or the equivalent for MIN_EXPR, *.c.004.original already has only one 
conditional left.
2019-07-01  Joern Rennecke  <joern.rennecke@riscy-ip.com>

	PR middle-end/66726
	* tree-ssa-phiopt.c (factor_out_conditional_conversion):
	Tune heuristic from PR71016 to allow MIN / MAX.
	* testsuite/gcc.dg/tree-ssa/pr66726-4.c: New testcase.

Index: tree-ssa-phiopt.c
===================================================================
--- tree-ssa-phiopt.c	(revision 272846)
+++ tree-ssa-phiopt.c	(working copy)
@@ -504,7 +504,25 @@ factor_out_conditional_conversion (edge
 		  gsi = gsi_for_stmt (arg0_def_stmt);
 		  gsi_prev_nondebug (&gsi);
 		  if (!gsi_end_p (gsi))
-		    return NULL;
+		    {
+		      if (gassign *assign
+			    = dyn_cast <gassign *> (gsi_stmt (gsi)))
+			{
+			  tree lhs = gimple_assign_lhs (assign);
+			  enum tree_code ass_code
+			    = gimple_assign_rhs_code (assign);
+			  if (ass_code != MAX_EXPR && ass_code != MIN_EXPR)
+			    return NULL;
+			  if (!operand_equal_p
+				 (lhs, gimple_assign_rhs1 (arg0_def_stmt), 0))
+			    return NULL;
+			  gsi_prev_nondebug (&gsi);
+			  if (!gsi_end_p (gsi))
+			    return NULL;
+			}
+		      else
+			return NULL;
+		    }
 		  gsi = gsi_for_stmt (arg0_def_stmt);
 		  gsi_next_nondebug (&gsi);
 		  if (!gsi_end_p (gsi))
Index: testsuite/gcc.dg/tree-ssa/pr66726-4.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/pr66726-4.c	(nonexistent)
+++ testsuite/gcc.dg/tree-ssa/pr66726-4.c	(working copy)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-phiopt1-details" } */
+
+#define SAT(x) (x < 0 ? 0 : (x > 255 ? 255 : x))
+
+void
+foo (unsigned char *p, int i)
+{
+  *p = SAT (i);
+}
+
+/* { dg-final { scan-tree-dump-times "COND_EXPR .*and PHI .*converted to straightline code" 1 "phiopt1" } } */
Richard Biener July 1, 2019, 4:28 p.m. | #3
On Mon, Jul 1, 2019 at 4:05 PM Joern Wolfgang Rennecke
<joern.rennecke@riscy-ip.com> wrote:
>

> [Apologies if this is a duplicate, I'm unsure if my previous mail was

> delivered]

> On 01/07/19 12:38, Richard Biener wrote:

> > On Mon, Jul 1, 2019 at 1:22 PM Joern Wolfgang Rennecke

> > <joern.rennecke@riscy-ip.com> wrote:

> >> The heuristic introduced for PR71016 prevents recognizing a max / min

> >> combo like it is used for

> >> saturation when followed by a conversion.

> >> The attached patch refines the heuristic to allow this case. Regression

> >> tested on x86_64-pc-linux-gnu .

> > Few style nits:

> >

> >    ...

> >

> > also please check that 'lhs' is equal to gimple_assing_rhs1 (arg0_def_stmt)

> > otherwise you'd also allow MIN/MAX unrelated to the conversion detected.

>

> Like the attached patch?


Yes - minor nit:

+                         if (!operand_equal_p
+                                (lhs, gimple_assign_rhs1 (arg0_def_stmt), 0))
+                           return NULL;

you can use

   if (lhs != gimple_assign_rhs1 (arg0_def_stmt))
     return NULL;

since SSA names are shared tree nodes.

OK with that change.

> >

> > On x86 I see the MIN_EXPR is already detected by GENERIC folding,

> > I wonder if that is required or if we can handle the case without in one

> > phiopt pass invocation as well.

> >

> tree_ssa_phiopt_worker is supposed to handle this by handling nested

> COND_EXPRs

> from the inside out:

>

>     /* Search every basic block for COND_EXPR we may be able to optimize.

>

>       We walk the blocks in order that guarantees that a block with

>       a single predecessor is processed before the predecessor.

>       This ensures that we collapse inner ifs before visiting the

>       outer ones, and also that we do not try to visit a removed

>       block.  */

>    bb_order = single_pred_before_succ_order ();

>

> However, I have no idea how to construct a testcase for this with the

> gimple folding in place.

>

> #define SAT(x) (x < 0 ? 0 : (x > 255 ? 255 : x))

>

> void

> foo (unsigned char *p, int i, int m)

> {

>    *p = (m > SAT (i) ? m : SAT (i));

> }

>

> or the equivalent for MIN_EXPR, *.c.004.original already has only one

> conditional left.


Hmm.  The testcase in your patch should be equal to

void
foo (unsigned char *p, int i)
{
  // tem1 = MAX ((unsinged char)MIN (i, 255), 0)
  unsigned char tem1;
  if (i >= 0)
    {
      // tem2 = MIN (i, 255)
      int tem2;
      if (i < 255)
        tem2 = i;
      else
        tem2 = 255;
      // tem1 = (unsigned char) tem2;
      tem1 = tem2;
    }
  else
    tem1 = 0;
  *p = tem1;
}

but for this I don't see any MIN/MAX recognition :/  Anyhow - probably
a stupid mistake on my side ;)

Richard.

>

Patch

Index: tree-ssa-phiopt.c
===================================================================
--- tree-ssa-phiopt.c	(revision 272846)
+++ tree-ssa-phiopt.c	(working copy)
@@ -504,7 +504,18 @@  factor_out_conditional_conversion (edge
 		  gsi = gsi_for_stmt (arg0_def_stmt);
 		  gsi_prev_nondebug (&gsi);
 		  if (!gsi_end_p (gsi))
-		    return NULL;
+		    {
+		      gimple *assign = gsi_stmt (gsi);
+		      if (gimple_code (assign) != GIMPLE_ASSIGN)
+			return NULL;
+		      tree lhs = gimple_assign_lhs (assign);
+		      enum tree_code ass_code = gimple_assign_rhs_code (assign);
+		      if (ass_code != MAX_EXPR && ass_code != MIN_EXPR)
+			return NULL;
+		      gsi_prev_nondebug (&gsi);
+		      if (!gsi_end_p (gsi))
+			return NULL;
+		    }
 		  gsi = gsi_for_stmt (arg0_def_stmt);
 		  gsi_next_nondebug (&gsi);
 		  if (!gsi_end_p (gsi))
Index: testsuite/gcc.dg/tree-ssa/pr66726-4.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/pr66726-4.c	(nonexistent)
+++ testsuite/gcc.dg/tree-ssa/pr66726-4.c	(working copy)
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-phiopt1-details" } */
+
+#define SAT(x) (x < 0 ? 0 : (x > 255 ? 255 : x))
+
+void
+foo (unsigned char *p, int i)
+{
+  *p = SAT (i);
+}
+
+/* { dg-final { scan-tree-dump-times "COND_EXPR .*and PHI .*converted to straightline code" 1 "phiopt1" } } */