[PATCH][RTL-ifcvt] PR rtl-optimization/67749: Do not emit separate SET insn in IF-ELSE case

[PATCH][RTL-ifcvt] PR rtl-optimization/67749: Do not emit separate SET insn in IF-ELSE case

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

Hi all,

This patch fixes the gcc.dg/ifcvt-2.c test for x86_64 where we were failing to if-convert.
This was because in my patch at https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228194
which tried to emit a SET to move the source of insn_a or insn_b (that came from the test block)
into a temporary. A SET however, is not always enough. For example, on x86_64 in order for the
resulting insn to be recognised it frequently needs to be in PARALLEL with a (clobber (reg:CC FLAGS_REG)).
This leads to the insn not being recognised.

So this patch removes that SET and instead generates a couple of temporary pseudos that gets passed
on a bit later to the code that loads the operands into registers when they're not general_operand.
This way we just modify the existing (recognisable) sets, allowing us to if-convert the testcase.

Bootstrapped and tested on x86_64, arm, aarch64.

Ok for trunk?

Thanks,
Kyrill

2015-10-27  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR rtl-optimization/67749
     * ifcvt.c (noce_try_cmove_arith): Do not emit move in IF-ELSE
     case before emitting the two blocks.  Instead modify the register
     in the corresponding final insn of the basic block.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ifcvt-x86-test.patch --]
[-- Type: text/x-patch; name=ifcvt-x86-test.patch, Size: 4916 bytes --]

commit d58740af39ceaf2d654cf3feff97b39fb6da3e82
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Sep 29 13:44:21 2015 +0100

    [RTL-ifcvt] PR rtl-optimization/67749

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 8846e69..7c6e7ca 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2135,38 +2135,29 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
      emit might clobber the register used by B or A, so move it to a pseudo
      first.  */
 
+  rtx tmp_a = NULL_RTX;
+  rtx tmp_b = NULL_RTX;
+
   if (b_simple || !else_bb)
-    {
-      rtx tmp_b = gen_reg_rtx (x_mode);
-      /* Perform the simplest kind of set.  The register allocator
-	 should remove it if it's not actually needed.  If this set is not
-	 a valid insn (can happen on the is_mem path) then end_ifcvt_sequence
-	 will cancel the whole sequence.  Don't try any of the fallback paths
-	 from noce_emit_move_insn since we want this to be the simplest kind
-	 of move.  */
-      emit_insn (gen_rtx_SET (tmp_b, b));
-      b = tmp_b;
-    }
+    tmp_b = gen_reg_rtx (x_mode);
 
   if (a_simple || !then_bb)
-    {
-      rtx tmp_a = gen_reg_rtx (x_mode);
-      emit_insn (gen_rtx_SET (tmp_a, a));
-      a = tmp_a;
-    }
+    tmp_a = gen_reg_rtx (x_mode);
 
   orig_a = a;
   orig_b = b;
 
   rtx emit_a = NULL_RTX;
   rtx emit_b = NULL_RTX;
-
+  rtx_insn *tmp_insn = NULL;
+  bool modified_in_a = false;
+  bool  modified_in_b = false;
   /* If either operand is complex, load it into a register first.
      The best way to do this is to copy the original insn.  In this
      way we preserve any clobbers etc that the insn may have had.
      This is of course not possible in the IS_MEM case.  */
 
-  if (! general_operand (a, GET_MODE (a)))
+  if (! general_operand (a, GET_MODE (a)) || tmp_a)
     {
 
       if (is_mem)
@@ -2174,36 +2165,51 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	  rtx reg = gen_reg_rtx (GET_MODE (a));
 	  emit_a = gen_rtx_SET (reg, a);
 	}
-      else if (! insn_a)
-	goto end_seq_and_fail;
       else
 	{
-	  a = gen_reg_rtx (GET_MODE (a));
-	  rtx_insn *copy_of_a = as_a <rtx_insn *> (copy_rtx (insn_a));
-	  rtx set = single_set (copy_of_a);
-	  SET_DEST (set) = a;
+	  if (insn_a)
+	    {
+	      a = tmp_a ? tmp_a : gen_reg_rtx (GET_MODE (a));
+
+	      rtx_insn *copy_of_a = as_a <rtx_insn *> (copy_rtx (insn_a));
+	      rtx set = single_set (copy_of_a);
+	      SET_DEST (set) = a;
 
-	  emit_a = PATTERN (copy_of_a);
+	      emit_a = PATTERN (copy_of_a);
+	    }
+	  else
+	    {
+	      rtx tmp_reg = tmp_a ? tmp_a : gen_reg_rtx (GET_MODE (a));
+	      emit_a = gen_rtx_SET (tmp_reg, a);
+	      a = tmp_reg;
+	    }
 	}
     }
 
-  if (! general_operand (b, GET_MODE (b)))
+  if (! general_operand (b, GET_MODE (b)) || tmp_b)
     {
       if (is_mem)
 	{
           rtx reg = gen_reg_rtx (GET_MODE (b));
 	  emit_b = gen_rtx_SET (reg, b);
 	}
-      else if (! insn_b)
-	goto end_seq_and_fail;
       else
 	{
-          b = gen_reg_rtx (GET_MODE (b));
-	  rtx_insn *copy_of_b = as_a <rtx_insn *> (copy_rtx (insn_b));
-	  rtx set = single_set (copy_of_b);
+	  if (insn_b)
+	    {
+	      b = tmp_b ? tmp_b : gen_reg_rtx (GET_MODE (b));
+	      rtx_insn *copy_of_b = as_a <rtx_insn *> (copy_rtx (insn_b));
+	      rtx set = single_set (copy_of_b);
 
-	  SET_DEST (set) = b;
-	  emit_b = PATTERN (copy_of_b);
+	      SET_DEST (set) = b;
+	      emit_b = PATTERN (copy_of_b);
+	    }
+	  else
+	    {
+	      rtx tmp_reg = tmp_b ? tmp_b : gen_reg_rtx (GET_MODE (b));
+	      emit_b = gen_rtx_SET (tmp_reg, b);
+	      b = tmp_reg;
+	  }
 	}
     }
 
@@ -2211,16 +2217,35 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
        swap insn that sets up A with the one that sets up B.  If even
        that doesn't help, punt.  */
 
-    if (emit_a && modified_in_p (orig_b, emit_a))
-      {
-	if (modified_in_p (orig_a, emit_b))
-	  goto end_seq_and_fail;
+  modified_in_a = emit_a != NULL_RTX && modified_in_p (orig_b, emit_a);
+  if (tmp_b && then_bb)
+    {
+      FOR_BB_INSNS (then_bb, tmp_insn)
+	if (modified_in_p (orig_b, tmp_insn))
+	  {
+	    modified_in_a = true;
+	    break;
+	  }
 
-	if (else_bb && !b_simple)
+    }
+    if (emit_a && modified_in_a)
+      {
+	modified_in_b = emit_b != NULL_RTX && modified_in_p (orig_a, emit_b);
+	if (tmp_b && else_bb)
 	  {
-	    if (!noce_emit_bb (emit_b, else_bb, b_simple))
-	      goto end_seq_and_fail;
+	    FOR_BB_INSNS (else_bb, tmp_insn)
+	      if (modified_in_p (orig_a, tmp_insn))
+		{
+		  modified_in_b = true;
+		  break;
+		}
+
 	  }
+	if (modified_in_b)
+	  goto end_seq_and_fail;
+
+	if (!noce_emit_bb (emit_b, else_bb, b_simple))
+	  goto end_seq_and_fail;
 
 	if (!noce_emit_bb (emit_a, then_bb, a_simple))
 	  goto end_seq_and_fail;

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67749: Do not emit separate SET insn in IF-ELSE case

[Jeff Law]

On 10/27/2015 08:49 AM, Kyrill Tkachov wrote:
> Hi all,
>
> This patch fixes the gcc.dg/ifcvt-2.c test for x86_64 where we were
> failing to if-convert. This was because in my patch at
> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228194 which
> tried to emit a SET to move the source of insn_a or insn_b (that came
> from the test block) into a temporary. A SET however, is not always
> enough. For example, on x86_64 in order for the resulting insn to be
> recognised it frequently needs to be in PARALLEL with a (clobber
> (reg:CC FLAGS_REG)). This leads to the insn not being recognised.
I don't think it affects the approach you've chosen, but I'm mentioning 
it for future reference.

gcse.c has some helper code (that probably ought to move into a more 
generic file) that will test for this situation.  Search for the 
instances of recog.  It essentially does something like

emit_insn (gen_rtx_SET (...))

And tries to recognize the result to determine if it's valid.


>
> So this patch removes that SET and instead generates a couple of
> temporary pseudos that gets passed on a bit later to the code that
> loads the operands into registers when they're not general_operand.
> This way we just modify the existing (recognisable) sets, allowing us
> to if-convert the testcase.
That sounds much more reasonable, assuming that the original 
destinations were just used once and those uses are guaranteed to be 
going away.



>
> Bootstrapped and tested on x86_64, arm, aarch64.
>
> Ok for trunk?

What happens in the case were noce_emit_bb returns a failure?  We've 
modified the original insns to use the new pseudos.  Won't this result 
in the original pseudo's uses using undefined values?

jeff

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67749: Do not emit separate SET insn in IF-ELSE case

[Kyrill Tkachov]

Hi Jeff,

On 02/11/15 22:46, Jeff Law wrote:
> On 10/27/2015 08:49 AM, Kyrill Tkachov wrote:
>> Hi all,
>>
>> This patch fixes the gcc.dg/ifcvt-2.c test for x86_64 where we were
>> failing to if-convert. This was because in my patch at
>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228194 which
>> tried to emit a SET to move the source of insn_a or insn_b (that came
>> from the test block) into a temporary. A SET however, is not always
>> enough. For example, on x86_64 in order for the resulting insn to be
>> recognised it frequently needs to be in PARALLEL with a (clobber
>> (reg:CC FLAGS_REG)). This leads to the insn not being recognised.
> I don't think it affects the approach you've chosen, but I'm mentioning it for future reference.
>
> gcse.c has some helper code (that probably ought to move into a more generic file) that will test for this situation.  Search for the instances of recog.  It essentially does something like
>
> emit_insn (gen_rtx_SET (...))
>
> And tries to recognize the result to determine if it's valid.

you mean compute_can_copy and can_copy_p? I was not aware of those. Interesting, they look like they
can be useful in places indeed. I'll keep them in mind for any future work.

>
>
>>
>> So this patch removes that SET and instead generates a couple of
>> temporary pseudos that gets passed on a bit later to the code that
>> loads the operands into registers when they're not general_operand.
>> This way we just modify the existing (recognisable) sets, allowing us
>> to if-convert the testcase.
> That sounds much more reasonable, assuming that the original destinations were just used once and those uses are guaranteed to be going away.
>
>
>
>>
>> Bootstrapped and tested on x86_64, arm, aarch64.
>>
>> Ok for trunk?
>
> What happens in the case were noce_emit_bb returns a failure? We've modified the original insns to use the new pseudos.  Won't this result in the original pseudo's uses using undefined values?

We should be fine because we don't modify the original insns. We create a copy of them i.e.
rtx_insn *copy_of_a = as_a <rtx_insn *> (copy_rtx (insn_a));

and modify the SET_DEST of that. The original insn should still remain intact if any step in
noce_try_cmove_arith fails, so we can revert back to the original sequence.

Thanks,
Kyrill

>
> jeff
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67749: Do not emit separate SET insn in IF-ELSE case

[Jeff Law]

On 11/03/2015 02:09 AM, Kyrill Tkachov wrote:
> Hi Jeff,
>
> On 02/11/15 22:46, Jeff Law wrote:
>> On 10/27/2015 08:49 AM, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> This patch fixes the gcc.dg/ifcvt-2.c test for x86_64 where we were
>>> failing to if-convert. This was because in my patch at
>>> https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=228194 which
>>> tried to emit a SET to move the source of insn_a or insn_b (that came
>>> from the test block) into a temporary. A SET however, is not always
>>> enough. For example, on x86_64 in order for the resulting insn to be
>>> recognised it frequently needs to be in PARALLEL with a (clobber
>>> (reg:CC FLAGS_REG)). This leads to the insn not being recognised.
>> I don't think it affects the approach you've chosen, but I'm
>> mentioning it for future reference.
>>
>> gcse.c has some helper code (that probably ought to move into a more
>> generic file) that will test for this situation.  Search for the
>> instances of recog.  It essentially does something like
>>
>> emit_insn (gen_rtx_SET (...))
>>
>> And tries to recognize the result to determine if it's valid.
>
> you mean compute_can_copy and can_copy_p? I was not aware of those.
> Interesting, they look like they
> can be useful in places indeed. I'll keep them in mind for any future work.
Yes.  Those.


>>
>> What happens in the case were noce_emit_bb returns a failure? We've
>> modified the original insns to use the new pseudos.  Won't this result
>> in the original pseudo's uses using undefined values?
>
> We should be fine because we don't modify the original insns. We create
> a copy of them i.e.
> rtx_insn *copy_of_a = as_a <rtx_insn *> (copy_rtx (insn_a));
>
> and modify the SET_DEST of that. The original insn should still remain
> intact if any step in
> noce_try_cmove_arith fails, so we can revert back to the original sequence.
In that case, OK for the trunk.

Thanks,
jeff