Re: [PATCH][RTL-ifcvt] Improve conditional select ops on immediates

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

On 03/08/15 18:37, Uros Bizjak wrote:
> On Mon, Aug 3, 2015 at 7:20 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>
> > > > > > Looking at the x86 movcc expansion code (ix86_expand_int_movcc) I
> > > > > > don't think this is a good idea. In the expander, there is already
> > > > > > quite some target-dependent code that goes great length to utilize sbb
> > > > > > insn as much as possible, before cmove is used.
> > > > > >
> > > > > > IMO, as far as x86 is concerned, the best solution would be to revert
> > > > > > the change. ix86_expand_int_movcc already does some tricks from your
> > > > > > patch in a target-efficient way. Generic change that was introduced by
> > > > > > your patch now interferes with this expansion.
> > > > > Well, technically the transformation was already there, it was just
> > > > > never
> > > > > reached for an x86 compilation because noce_try_cmove was tried in front
> > > > > of
> > > > > it
> > > > > and used a target-specific expansion.
> > > > > In any case, how's this proposal?
> > > > > The transformation noce_try_store_flag_constants
> > > > >          /* if (test) x = a; else x = b;
> > > > >         =>   x = (-(test != 0) & (b - a)) + a;  */
> > > > >
> > > > > Is a catch-all-immediates transformation in
> > > > > noce_try_store_flag_constants.
> > > > > What if we moved it to noce_try_cmove and performed it only if the
> > > > > target-specific
> > > > > conditional move expansion there failed?
> > > > >
> > > > > That way we can try the x86_64-specific sequence first and still give
> > > > > the
> > > > > opportunity
> > > > > to noce_try_store_flag_constants to perform the transformations that can
> > > > > benefit targets
> > > > > that don't have highly specific conditional move expanders.
> > > > Yes, let's try this approach. As was found out, some targets (e.g.
> > > > x86) hide lots of different target-dependent expansion strategies into
> > > > movcc expander. Perhaps this fact should be documented in the comment
> > > > in the generic code?
> > > Ok, I'll work on that approach and add a comment.
> >
> > I'm testing a patch that fix the testcases on x86_64 and does not
> > harm codegen on aarch64. Feel free to file a PR and assign it to me.
> PR67103 [1]
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67103

Thanks,
Here's the patch to move that transformation from noce_try_store_flag_constants
to noce_try_cmove after the target-specific expansion has had a go.

This fixes the testcases for me on x86_64.
In i386.exp I only see:
FAIL: gcc.target/i386/pr49781-1.c scan-assembler-not lea[lq]?[ \t]\\((%|)r[a-z0-9]*
FAIL: gcc.target/i386/pr61403.c scan-assembler blend

which were there before my patch.
Bootstrap and testing on x86_64, arm and aarch64 is successful for me.

Is this ok?

Thanks,
Kyrill

2015-08-04  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

    PR rtl-optimization/67103
    * ifcvt.c (noce_try_store_flag_constants): Move
    x = (-(test != 0) & (b - a)) + a transformation to...
    (noce_try_cmove): ... Here.  Try it if normal conditional
    move fails.


> Thanks,
> Uros.

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 0ebe107..b06eaab 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1410,9 +1410,6 @@ noce_try_store_flag_constants (struct noce_if_info *if_info)
 	  normalize = -1;
 	  reversep = true;
 	}
-      else if ((if_info->branch_cost >= 2 && STORE_FLAG_VALUE == -1)
-	       || if_info->branch_cost >= 3)
-	normalize = -1;
       else
 	return FALSE;
 
@@ -1481,18 +1478,10 @@ noce_try_store_flag_constants (struct noce_if_info *if_info)
 					target, gen_int_mode (ifalse, mode),
 					if_info->x, 0, OPTAB_WIDEN);
 	}
-
-      /* if (test) x = a; else x = b;
-	 =>   x = (-(test != 0) & (b - a)) + a;  */
       else
 	{
-	  target = expand_simple_binop (mode, AND,
-					target, gen_int_mode (diff, mode),
-					if_info->x, 0, OPTAB_WIDEN);
-	  if (target)
-	    target = expand_simple_binop (mode, PLUS,
-					  target, gen_int_mode (ifalse, mode),
-					  if_info->x, 0, OPTAB_WIDEN);
+	  end_sequence ();
+	  return FALSE;
 	}
 
       if (! target)
@@ -1818,11 +1807,67 @@ noce_try_cmove (struct noce_if_info *if_info)
 				   INSN_LOCATION (if_info->insn_a));
 	  return TRUE;
 	}
-      else
+      /* If both a and b are constants try a last-ditch transformation:
+	 if (test) x = a; else x = b;
+	 =>   x = (-(test != 0) & (b - a)) + a;
+	 Try this only if the target-specific expansion above has failed.
+	 The target-specific expander may want to generate sequences that
+	 we don't know about, so give them a chance before trying this
+	 approach.  */
+      else if (!targetm.have_conditional_execution ()
+		&& CONST_INT_P (if_info->a) && CONST_INT_P (if_info->b)
+		&& ((if_info->branch_cost >= 2 && STORE_FLAG_VALUE == -1)
+		    || if_info->branch_cost >= 3))
 	{
-	  end_sequence ();
-	  return FALSE;
+	  machine_mode mode = GET_MODE (if_info->x);
+	  HOST_WIDE_INT ifalse = INTVAL (if_info->a);
+	  HOST_WIDE_INT itrue = INTVAL (if_info->b);
+	  rtx target = noce_emit_store_flag (if_info, if_info->x, false, -1);
+	  if (!target)
+	    {
+	      end_sequence ();
+	      return FALSE;
+	    }
+
+	  HOST_WIDE_INT diff = (unsigned HOST_WIDE_INT) itrue - ifalse;
+	  /* Make sure we can represent the difference
+	     between the two values.  */
+	  if ((diff > 0)
+	      != ((ifalse < 0) != (itrue < 0) ? ifalse < 0 : ifalse < itrue))
+	    {
+	      end_sequence ();
+	      return FALSE;
+	    }
+
+	  diff = trunc_int_for_mode (diff, mode);
+	  target = expand_simple_binop (mode, AND,
+					target, gen_int_mode (diff, mode),
+					if_info->x, 0, OPTAB_WIDEN);
+	  if (target)
+	    target = expand_simple_binop (mode, PLUS,
+					  target, gen_int_mode (ifalse, mode),
+					  if_info->x, 0, OPTAB_WIDEN);
+	  if (target)
+	    {
+	      if (target != if_info->x)
+		noce_emit_move_insn (if_info->x, target);
+
+	      seq = end_ifcvt_sequence (if_info);
+	      if (!seq)
+		return FALSE;
+
+	      emit_insn_before_setloc (seq, if_info->jump,
+				   INSN_LOCATION (if_info->insn_a));
+	      return TRUE;
+	    }
+	  else
+	    {
+	      end_sequence ();
+	      return FALSE;
+	    }
 	}
+      else
+	end_sequence ();
     }
 
   return FALSE;