diff mbox

[optabs,ifcvt,1/3] Define negcc, notcc optabs

Message ID 55E6FD10.9050502@arm.com
State Superseded
Headers show

Commit Message

Kyrylo Tkachov Sept. 2, 2015, 1:43 p.m. UTC
Hi Jeff,

On 01/09/15 23:13, Jeff Law wrote:
> On 09/01/2015 09:04 AM, Kyrill Tkachov wrote:
>> Hi all,
>>
>> This first patch introduces the negcc and notcc optabs that should
>> expand to a conditional
>> negate or a conditional bitwise complement operation.
>>
>> These are used in ifcvt.c to transform code of the form:
>> if (test) x = -A; else x = A;
>> into:
>> x = A; if (test) x = -x;
>> where the "if (test) x = -x;" is implemented using the negcc (or notcc
>> in the ~x case)
>> if such an optab is available. If such an optab is not implemented then
>> no transformation
>> is performed.  Thus, without patches 2/3 and 3/3 this patch does not
>> impact behaviour on any target.
>>
>> Bootstrapped and tested as part of the series on arm, aarch64, x86_64.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Kyrill
>>
>> 2015-09-01  Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>>
>>       * ifcvt.c (noce_try_inverse_constants): New function.
>>       (noce_process_if_block): Call it.
>>       * optabs.h (emit_conditional_neg_or_complement): Declare prototype.
>>       * optabs.def (negcc_optab, notcc_optab): Declare.
>>       * optabs.c (emit_conditional_neg_or_complement): New function.
>>       * doc/tm.texi (Standard Names): Document negcc, notcc names.
>>
>> negnotcc-optabs.patch
>>
>>
>> commit a2183218070ed5f2dca0a9651fdb08ce134ba8ee
>> Author: Kyrylo Tkachov<kyrylo.tkachov@arm.com>
>> Date:   Thu Aug 13 18:14:52 2015 +0100
>>
>>       [optabs][ifcvt][1/3] Define negcc, notcc optabs
>>
>> diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
>> index 0bffdc6..5038269 100644
>> --- a/gcc/doc/md.texi
>> +++ b/gcc/doc/md.texi
>> @@ -5791,6 +5791,21 @@ move operand 2 or (operands 2 + operand 3) into operand 0 according to the
>>    comparison in operand 1.  If the comparison is false, operand 2 is moved into
>>    operand 0, otherwise (operand 2 + operand 3) is moved.
>>
>> +@cindex @code{neg@var{mode}cc} instruction pattern
>> +@item @samp{neg@var{mode}cc}
>> +Similar to @samp{mov@var{mode}cc} but for conditional negation.  Conditionally
>> +move the negation of operand 2 operand 3 into operand 0 according to the
>> +comparison in operand 1.  If the comparison is true, the negation of operand 2
>> +is moved into operand 0, otherwise operand 3 is moved.
>> +
>> +@cindex @code{not@var{mode}cc} instruction pattern
>> +@item @samp{not@var{mode}cc}
>> +Similar to @samp{neg@var{mode}cc} but for conditional complement.
>> +Conditionally move the bitwise complement of operand 2 operand 3 into operand 0
>> +according to the comparison in operand 1.  If the comparison is true,
>> +the complement of operand 2 is moved into operand 0, otherwise operand 3 is
>> +moved.
> "operand 2 operand 3" does not parse.  I think you mean "operand 2 or
> operand 3" in both instances above.  Even that doesn't parse well as
> it's not clear if operand3 or the negation/complement of operand 3 is
> meant.  Can you try to improve the ambiguity of the second sentence in
> each description.

You're right, it doesn't parse. I've improved the description.
We either move the negated operand 2 or the unchanged operand 3.

>
> And just a note.  The canonical way to refer to these patterns should be
> negcc/notcc.  That avoids conflicting with the older negscc patterns
> with different semantics that are defined by some ports.  You're already
> using that terminology, so there's nothing to change, I just wanted to
> point it out.
>
>
>
> +
>> +  rtx_code code;
>> +  if (val_a == -val_b)
> Do we have to worry about signed overflow here?  I'm thinking
> specifically when val_b is the smallest possible integer representable
> by a HOST_WIDE_INT.  I suspect you may be able to avoid these problems
> with judicious use of the hwi interfaces.

I understand the issue, but am not sure what hwi interfaces to use here.
Seems that the problem will be if val_b is HOST_WIDE_INT_MIN.
Looking at the definition of abs_hwi in hwint.h before it negates it's argument
it asserts that it's not HOST_WIDE_INT_MIN. I think that's to avoid this exact issue?
If so, I've added a check for HOST_WIDE_INT_MIN which should cover the undefined case
when negating a HOST_WIDE_INT, unless there's something else I'm missing.

>
>
> So I think we just need to resolve the doc change and make sure we're
> not triggering undefined behaviour above and this can go forward.

Thanks, here's the updated patch.
Kyrill

2015-09-02  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

      * ifcvt.c (noce_try_inverse_constants): New function.
      (noce_process_if_block): Call it.
      * optabs.h (emit_conditional_neg_or_complement): Declare prototype.
      * optabs.def (negcc_optab, notcc_optab): Declare.
      * optabs.c (emit_conditional_neg_or_complement): New function.
      * doc/tm.texi (Standard Names): Document negcc, notcc names.


> jeff
>

Comments

Jeff Law Sept. 9, 2015, 9:51 p.m. UTC | #1
On 09/02/2015 07:43 AM, Kyrill Tkachov wrote:

>>> +  rtx_code code; +  if (val_a == -val_b)
>> Do we have to worry about signed overflow here?  I'm thinking
>> specifically when val_b is the smallest possible integer
>> representable by a HOST_WIDE_INT.  I suspect you may be able to
>> avoid these problems with judicious use of the hwi interfaces.
>
> I understand the issue, but am not sure what hwi interfaces to use
> here. Seems that the problem will be if val_b is HOST_WIDE_INT_MIN.
Right.  When val_b is HOST_WIDE_INT_MIN, negating it will overflow.

> Looking at the definition of abs_hwi in hwint.h before it negates
> it's argument it asserts that it's not HOST_WIDE_INT_MIN. I think
> that's to avoid this exact issue? If so, I've added a check for
> HOST_WIDE_INT_MIN which should cover the undefined case when negating
> a HOST_WIDE_INT, unless there's something else I'm missing.
That should be sufficient.

2015-09-02  Kyrylo Tkachov <kyrylo.tkachov@arm.com>

      * ifcvt.c (noce_try_inverse_constants): New function.
      (noce_process_if_block): Call it.
      * optabs.h (emit_conditional_neg_or_complement): Declare prototype.
      * optabs.def (negcc_optab, notcc_optab): Declare.
      * optabs.c (emit_conditional_neg_or_complement): New function.
      * doc/tm.texi (Standard Names): Document negcc, notcc names.

OK for the trunk.

Thanks for your patience.

Jeff
Kyrylo Tkachov Sept. 10, 2015, 8:23 a.m. UTC | #2
On 09/09/15 22:51, Jeff Law wrote:
> On 09/02/2015 07:43 AM, Kyrill Tkachov wrote:
>
>>>> +  rtx_code code; +  if (val_a == -val_b)
>>> Do we have to worry about signed overflow here?  I'm thinking
>>> specifically when val_b is the smallest possible integer
>>> representable by a HOST_WIDE_INT.  I suspect you may be able to
>>> avoid these problems with judicious use of the hwi interfaces.
>> I understand the issue, but am not sure what hwi interfaces to use
>> here. Seems that the problem will be if val_b is HOST_WIDE_INT_MIN.
> Right.  When val_b is HOST_WIDE_INT_MIN, negating it will overflow.
>
>> Looking at the definition of abs_hwi in hwint.h before it negates
>> it's argument it asserts that it's not HOST_WIDE_INT_MIN. I think
>> that's to avoid this exact issue? If so, I've added a check for
>> HOST_WIDE_INT_MIN which should cover the undefined case when negating
>> a HOST_WIDE_INT, unless there's something else I'm missing.
> That should be sufficient.
>
> 2015-09-02  Kyrylo Tkachov <kyrylo.tkachov@arm.com>
>
>        * ifcvt.c (noce_try_inverse_constants): New function.
>        (noce_process_if_block): Call it.
>        * optabs.h (emit_conditional_neg_or_complement): Declare prototype.
>        * optabs.def (negcc_optab, notcc_optab): Declare.
>        * optabs.c (emit_conditional_neg_or_complement): New function.
>        * doc/tm.texi (Standard Names): Document negcc, notcc names.
>
> OK for the trunk.

Thanks for the review.
I'll commit it if/when the aarch64 and arm implementations of the hooks
in patches 2/3 and 3/3 are approved.

Kyrill

>
> Thanks for your patience.
>
> Jeff
>
diff mbox

Patch

commit e3546b6e9fa772fe15f0a9845dbe429c02f5b327
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Aug 13 18:14:52 2015 +0100

    [optabs][ifcvt][1/3] Define negcc, notcc optabs

diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 619259f..c4e43f3 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -5791,6 +5791,21 @@  move operand 2 or (operands 2 + operand 3) into operand 0 according to the
 comparison in operand 1.  If the comparison is false, operand 2 is moved into
 operand 0, otherwise (operand 2 + operand 3) is moved.
 
+@cindex @code{neg@var{mode}cc} instruction pattern
+@item @samp{neg@var{mode}cc}
+Similar to @samp{mov@var{mode}cc} but for conditional negation.  Conditionally
+move the negation of operand 2 or the unchanged operand 3 into operand 0
+according to the comparison in operand 1.  If the comparison is true, the negation
+of operand 2 is moved into operand 0, otherwise operand 3 is moved.
+
+@cindex @code{not@var{mode}cc} instruction pattern
+@item @samp{not@var{mode}cc}
+Similar to @samp{neg@var{mode}cc} but for conditional complement.
+Conditionally move the bitwise complement of operand 2 or the unchanged
+operand 3 into operand 0 according to the comparison in operand 1.
+If the comparison is true, the complement of operand 2 is moved into
+operand 0, otherwise operand 3 is moved.
+
 @cindex @code{cstore@var{mode}4} instruction pattern
 @item @samp{cstore@var{mode}4}
 Store zero or nonzero in operand 0 according to whether a comparison
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 157a716..d2f5b66 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1179,6 +1179,88 @@  noce_try_store_flag (struct noce_if_info *if_info)
     }
 }
 
+
+/* Convert "if (test) x = -A; else x = A" into
+   x = A; if (test) x = -x if the machine can do the
+   conditional negate form of this cheaply.
+   Try this before noce_try_cmove that will just load the
+   immediates into two registers and do a conditional select
+   between them.  If the target has a conditional negate or
+   conditional invert operation we can save a potentially
+   expensive constant synthesis.  */
+
+static bool
+noce_try_inverse_constants (struct noce_if_info *if_info)
+{
+  if (!noce_simple_bbs (if_info))
+    return false;
+
+  if (!CONST_INT_P (if_info->a)
+      || !CONST_INT_P (if_info->b)
+      || !REG_P (if_info->x))
+    return false;
+
+  machine_mode mode = GET_MODE (if_info->x);
+
+  /* If the branch is cheaper than two instructions then this is
+     unlikely to be beneficial.  */
+  if (if_info->branch_cost < 2)
+    return false;
+
+  HOST_WIDE_INT val_a = INTVAL (if_info->a);
+  HOST_WIDE_INT val_b = INTVAL (if_info->b);
+
+  rtx cond = if_info->cond;
+
+  rtx x = if_info->x;
+  rtx target;
+
+  start_sequence ();
+
+  rtx_code code;
+  if (val_b != HOST_WIDE_INT_MIN && val_a == -val_b)
+    code = NEG;
+  else if (val_a == ~val_b)
+    code = NOT;
+  else
+    {
+      end_sequence ();
+      return false;
+    }
+
+  rtx tmp = gen_reg_rtx (mode);
+  noce_emit_move_insn (tmp, if_info->a);
+
+  target = emit_conditional_neg_or_complement (x, code, mode, cond, tmp, tmp);
+
+  if (target)
+    {
+      rtx_insn *seq = get_insns ();
+
+      if (!seq)
+	{
+	  end_sequence ();
+	  return false;
+	}
+
+      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;
+    }
+
+  end_sequence ();
+  return false;
+}
+
+
 /* Convert "if (test) x = a; else x = b", for A and B constant.
    Also allow A = y + c1, B = y + c2, with a common y between A
    and B.  */
@@ -3190,6 +3272,8 @@  noce_process_if_block (struct noce_if_info *if_info)
     goto success;
   if (noce_try_abs (if_info))
     goto success;
+  if (noce_try_inverse_constants (if_info))
+    goto success;
   if (!targetm.have_conditional_execution ()
       && noce_try_store_flag_constants (if_info))
     goto success;
diff --git a/gcc/optabs.c b/gcc/optabs.c
index e533e6e..aad9f88 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -4597,6 +4597,56 @@  emit_conditional_move (rtx target, enum rtx_code code, rtx op0, rtx op1,
   return NULL_RTX;
 }
 
+
+/* Emit a conditional negate or bitwise complement using the
+   negcc or notcc optabs if available.  Return NULL_RTX if such operations
+   are not available.  Otherwise return the RTX holding the result.
+   TARGET is the desired destination of the result.  COMP is the comparison
+   on which to negate.  If COND is true move into TARGET the negation
+   or bitwise complement of OP1.  Otherwise move OP2 into TARGET.
+   CODE is either NEG or NOT.  MODE is the machine mode in which the
+   operation is performed.  */
+
+rtx
+emit_conditional_neg_or_complement (rtx target, rtx_code code,
+				     machine_mode mode, rtx cond, rtx op1,
+				     rtx op2)
+{
+  optab op = unknown_optab;
+  if (code == NEG)
+    op = negcc_optab;
+  else if (code == NOT)
+    op = notcc_optab;
+  else
+    gcc_unreachable ();
+
+  insn_code icode = direct_optab_handler (op, mode);
+
+  if (icode == CODE_FOR_nothing)
+    return NULL_RTX;
+
+  if (!target)
+    target = gen_reg_rtx (mode);
+
+  rtx_insn *last = get_last_insn ();
+  struct expand_operand ops[4];
+
+  create_output_operand (&ops[0], target, mode);
+  create_fixed_operand (&ops[1], cond);
+  create_input_operand (&ops[2], op1, mode);
+  create_input_operand (&ops[3], op2, mode);
+
+  if (maybe_expand_insn (icode, 4, ops))
+    {
+      if (ops[0].value != target)
+	convert_move (target, ops[0].value, false);
+
+      return target;
+    }
+  delete_insns_since (last);
+  return NULL_RTX;
+}
+
 /* Return nonzero if a conditional move of mode MODE is supported.
 
    This function is for combine so it can tell whether an insn that looks
diff --git a/gcc/optabs.def b/gcc/optabs.def
index 888b21c..6fad6d9 100644
--- a/gcc/optabs.def
+++ b/gcc/optabs.def
@@ -183,6 +183,8 @@  OPTAB_D (reload_out_optab, "reload_out$a")
 
 OPTAB_DC(cbranch_optab, "cbranch$a4", COMPARE)
 OPTAB_D (addcc_optab, "add$acc")
+OPTAB_D (negcc_optab, "neg$acc")
+OPTAB_D (notcc_optab, "not$acc")
 OPTAB_D (movcc_optab, "mov$acc")
 OPTAB_D (cmov_optab, "cmov$a6")
 OPTAB_D (cstore_optab, "cstore$a4")
diff --git a/gcc/optabs.h b/gcc/optabs.h
index 95f5cbc..dbb73d1 100644
--- a/gcc/optabs.h
+++ b/gcc/optabs.h
@@ -368,6 +368,10 @@  extern void emit_indirect_jump (rtx);
 rtx emit_conditional_move (rtx, enum rtx_code, rtx, rtx, machine_mode,
 			   rtx, rtx, machine_mode, int);
 
+/* Emit a conditional negate or bitwise complement operation.  */
+rtx emit_conditional_neg_or_complement (rtx, rtx_code, machine_mode, rtx,
+					 rtx, rtx);
+
 /* Return nonzero if the conditional move is supported.  */
 int can_conditionally_move_p (machine_mode mode);