[PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

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

Hi all,

This is the second attempt to fix the PRs.
The first one at https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00449.html
does the trick, but is overly restrictive.

This one allows for cases when one block is complex and the other one is simple or empty.
Earlier, this case would bypass the bbs_ok_for_cmove_arith and we would end up
if-converting cases where the 'else' part (x := b) was pulled from the test block
earlier in the call chain. If the reg 'b' in this case was also written to by the
'then' block, then we would miscompile.

With this patch we move the original 'b' value into a pseudo before potentially clobbering
it, so all is wired up properly.

With this patch the PRs work for me and the restriction on if-conversion is not overly aggressive.
Rainer, could you please check that this patch still fixes the SPARC regressions?

I've bootstrapped and tested this on x86_64 and aarch64.

Ok for trunk if SPARC testing is fine?

Thanks,
Kyrill

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

     PR rtl-optimization/67456
     PR rtl-optimization/67464
     PR rtl-optimization/67465
     PR rtl-optimization/67481
     * ifcvt.c (noce_try_cmove_arith): Bail out if cannot conditionally
     move in the mode of x.  Handle combination of complex and simple
     block pairs as well as the case when one is empty.

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

     * gcc.dg/pr67465.c: New test.

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

commit ca25fa7000dd9d086b78bf9a02126f83fbab8073
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Sep 7 14:58:01 2015 +0100

    [RTL-ifcvt] PR rtl-optimization/67465: Do not ifcvt complex blocks if the else block is empty

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index d2f5b66..9adce60 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1997,6 +1997,7 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
   rtx a = if_info->a;
   rtx b = if_info->b;
   rtx x = if_info->x;
+  machine_mode x_mode = GET_MODE (x);
   rtx orig_a, orig_b;
   rtx_insn *insn_a, *insn_b;
   bool a_simple = if_info->then_simple;
@@ -2008,6 +2009,9 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
   enum rtx_code code;
   rtx_insn *ifcvt_seq;
 
+  if (!can_conditionally_move_p (x_mode))
+    return FALSE;
+
   /* A conditional move from two memory sources is equivalent to a
      conditional on their addresses followed by a load.  Don't do this
      early because it'll screw alias analysis.  Note that we've
@@ -2079,13 +2083,32 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	}
     }
 
-  if (!a_simple && then_bb && !b_simple && else_bb
+  if (then_bb && else_bb && !a_simple && !b_simple
       && (!bbs_ok_for_cmove_arith (then_bb, else_bb)
 	  || !bbs_ok_for_cmove_arith (else_bb, then_bb)))
     return FALSE;
 
   start_sequence ();
 
+  /* If one of the blocks is empty then the corresponding B or A value
+     came from the test block.  The non-empty complex block that we will
+     emit might clobber the register used by B or A, so move it to a pseudo
+     first.  */
+
+  if (b_simple || !else_bb)
+    {
+      rtx tmp_b = gen_reg_rtx (x_mode);
+      noce_emit_move_insn (tmp_b, b);
+      b = tmp_b;
+    }
+
+  if (a_simple || !then_bb)
+    {
+      rtx tmp_a = gen_reg_rtx (x_mode);
+      noce_emit_move_insn (tmp_a, a);
+      a = tmp_a;
+    }
+
   orig_a = a;
   orig_b = b;
 
diff --git a/gcc/testsuite/gcc.dg/pr67465.c b/gcc/testsuite/gcc.dg/pr67465.c
new file mode 100644
index 0000000..321fd38
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr67465.c
@@ -0,0 +1,53 @@
+/* { dg-do run } */
+/* { dg-options "-O3 -std=gnu99" } */
+
+int a, b, c, d, e, h;
+
+int
+fn1 (int p1)
+{
+  {
+    int g[2];
+    for (int i = 0; i < 1; i++)
+      g[i] = 0;
+    if (g[0] < c)
+      {
+	a = (unsigned) (1 ^ p1) % 2;
+	return 0;
+      }
+  }
+  return 0;
+}
+
+void
+fn2 ()
+{
+  for (h = 0; h < 1; h++)
+    {
+      for (int j = 0; j < 2; j++)
+	{
+	  for (b = 1; b; b = 0)
+	    a = 1;
+	  for (; b < 1; b++)
+	    ;
+	  if (e)
+	    continue;
+	  a = 2;
+	}
+      fn1 (h);
+      short k = -16;
+      d = k > a;
+    }
+}
+
+int
+main ()
+{
+  fn2 ();
+
+  if (a != 2)
+    __builtin_abort ();
+
+  return 0;
+}
+

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Rainer Orth]

Hi Kyrill,

> Rainer, could you please check that this patch still fixes the SPARC
> regressions?

unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
stage2 libiberty/regex.c FAILs:

In file included from /vol/gcc/src/hg/trunk/local/libiberty/regex.c:640:0:
/vol/gcc/src/hg/trunk/local/libiberty/regex.c: In function 'byte_regex_compile':
/vol/gcc/src/hg/trunk/local/libiberty/regex.c:4223:1: internal compiler error: in emit_move_insn, at expr.c:3552
 } /* regex_compile */
 ^
0x6f1893 emit_move_insn(rtx_def*, rtx_def*)
        /vol/gcc/src/hg/trunk/local/gcc/expr.c:3551
0x13c524f noce_emit_move_insn
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:927
0x13c8d93 noce_try_cmove_arith
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:2026
0x13cd40b noce_process_if_block
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:3229
0x13ceeb7 noce_find_if_block
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:3678
0x13cf8fb find_if_header
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:3883
0x13d39df if_convert
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:5030
0x13d3f13 execute
        /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:5176

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

Hi Rainer,

On 10/09/15 12:43, Rainer Orth wrote:
> Hi Kyrill,
>
>> Rainer, could you please check that this patch still fixes the SPARC
>> regressions?
> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
> stage2 libiberty/regex.c FAILs:

Thanks for trying it out.
I'll try reproducing with a stage-1 cross-compiler.
Any change you could send me the preprocessed regex.i?

Thanks,
Kyrill


> In file included from /vol/gcc/src/hg/trunk/local/libiberty/regex.c:640:0:
> /vol/gcc/src/hg/trunk/local/libiberty/regex.c: In function 'byte_regex_compile':
> /vol/gcc/src/hg/trunk/local/libiberty/regex.c:4223:1: internal compiler error: in emit_move_insn, at expr.c:3552
>   } /* regex_compile */
>   ^
> 0x6f1893 emit_move_insn(rtx_def*, rtx_def*)
>          /vol/gcc/src/hg/trunk/local/gcc/expr.c:3551
> 0x13c524f noce_emit_move_insn
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:927
> 0x13c8d93 noce_try_cmove_arith
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:2026
> 0x13cd40b noce_process_if_block
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:3229
> 0x13ceeb7 noce_find_if_block
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:3678
> 0x13cf8fb find_if_header
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:3883
> 0x13d39df if_convert
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:5030
> 0x13d3f13 execute
>          /vol/gcc/src/hg/trunk/local/gcc/ifcvt.c:5176
>
> 	Rainer
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

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


On 10/09/15 12:43, Rainer Orth wrote:
> Hi Kyrill,
>
>> Rainer, could you please check that this patch still fixes the SPARC
>> regressions?
> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
> stage2 libiberty/regex.c FAILs:
>
>

Thanks for providing the preprocessed file.
I've reproduced and fixed the ICE in this version of the patch.
The problem was that I was taking the mode of x before the check
of whether a and b are MEMs, after which we would change x to an address_mode reg,
thus confusing emit_move_insn.

The fix is to take the mode of x and perform the can_conditionally_move_p check
after that transformation.

Bootstrapped and tested on aarch64 and x86_64.
The preprocessed regex.i that Rainer provided now compiles successfully for me
on a sparc-sun-solaris2.10 stage-1 cross-compiler.

Rainer, thanks for your help so far, could you please try out this patch?

Thanks,
Kyrill

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

      PR rtl-optimization/67456
      PR rtl-optimization/67464
      PR rtl-optimization/67465
      PR rtl-optimization/67481
      * ifcvt.c (noce_try_cmove_arith): Bail out if cannot conditionally
      move in the mode of x.  Handle combination of complex and simple
      block pairs as well as the case when one is empty.

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

      * gcc.dg/pr67465.c: New test.

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

commit 90c12de20cdc8a8a5e81c25accc640ab517a3f7c
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Sep 7 14:58:01 2015 +0100

    [RTL-ifcvt] PR rtl-optimization/67465: Do not ifcvt complex blocks if the else block is empty

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index d2f5b66..e708ac4 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2043,6 +2043,11 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
   insn_a = if_info->insn_a;
   insn_b = if_info->insn_b;
 
+  machine_mode x_mode = GET_MODE (x);
+
+  if (!can_conditionally_move_p (x_mode))
+    return FALSE;
+
   unsigned int then_cost;
   unsigned int else_cost;
   if (insn_a)
@@ -2079,13 +2084,32 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	}
     }
 
-  if (!a_simple && then_bb && !b_simple && else_bb
+  if (then_bb && else_bb && !a_simple && !b_simple
       && (!bbs_ok_for_cmove_arith (then_bb, else_bb)
 	  || !bbs_ok_for_cmove_arith (else_bb, then_bb)))
     return FALSE;
 
   start_sequence ();
 
+  /* If one of the blocks is empty then the corresponding B or A value
+     came from the test block.  The non-empty complex block that we will
+     emit might clobber the register used by B or A, so move it to a pseudo
+     first.  */
+
+  if (b_simple || !else_bb)
+    {
+      rtx tmp_b = gen_reg_rtx (x_mode);
+      noce_emit_move_insn (tmp_b, b);
+      b = tmp_b;
+    }
+
+  if (a_simple || !then_bb)
+    {
+      rtx tmp_a = gen_reg_rtx (x_mode);
+      noce_emit_move_insn (tmp_a, a);
+      a = tmp_a;
+    }
+
   orig_a = a;
   orig_b = b;
 
diff --git a/gcc/testsuite/gcc.dg/pr67465.c b/gcc/testsuite/gcc.dg/pr67465.c
new file mode 100644
index 0000000..321fd38
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr67465.c
@@ -0,0 +1,53 @@
+/* { dg-do run } */
+/* { dg-options "-O3 -std=gnu99" } */
+
+int a, b, c, d, e, h;
+
+int
+fn1 (int p1)
+{
+  {
+    int g[2];
+    for (int i = 0; i < 1; i++)
+      g[i] = 0;
+    if (g[0] < c)
+      {
+	a = (unsigned) (1 ^ p1) % 2;
+	return 0;
+      }
+  }
+  return 0;
+}
+
+void
+fn2 ()
+{
+  for (h = 0; h < 1; h++)
+    {
+      for (int j = 0; j < 2; j++)
+	{
+	  for (b = 1; b; b = 0)
+	    a = 1;
+	  for (; b < 1; b++)
+	    ;
+	  if (e)
+	    continue;
+	  a = 2;
+	}
+      fn1 (h);
+      short k = -16;
+      d = k > a;
+    }
+}
+
+int
+main ()
+{
+  fn2 ();
+
+  if (a != 2)
+    __builtin_abort ();
+
+  return 0;
+}
+

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Rainer Orth]

Kyrill Tkachov <kyrylo.tkachov@arm.com> writes:

> On 10/09/15 12:43, Rainer Orth wrote:
>> Hi Kyrill,
>>
>>> Rainer, could you please check that this patch still fixes the SPARC
>>> regressions?
>> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
>> stage2 libiberty/regex.c FAILs:
>>
>>
>
> Thanks for providing the preprocessed file.
> I've reproduced and fixed the ICE in this version of the patch.
> The problem was that I was taking the mode of x before the check
> of whether a and b are MEMs, after which we would change x to an
> address_mode reg,
> thus confusing emit_move_insn.
>
> The fix is to take the mode of x and perform the can_conditionally_move_p check
> after that transformation.
>
> Bootstrapped and tested on aarch64 and x86_64.
> The preprocessed regex.i that Rainer provided now compiles successfully for me
> on a sparc-sun-solaris2.10 stage-1 cross-compiler.
>
> Rainer, thanks for your help so far, could you please try out this patch?

While bootstrap succeeds again, the testsuite regression in
gcc.c-torture/execute/20071216-1.c reoccured.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

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


On 11/09/15 09:51, Rainer Orth wrote:
> Kyrill Tkachov <kyrylo.tkachov@arm.com> writes:
>
>> On 10/09/15 12:43, Rainer Orth wrote:
>>> Hi Kyrill,
>>>
>>>> Rainer, could you please check that this patch still fixes the SPARC
>>>> regressions?
>>> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
>>> stage2 libiberty/regex.c FAILs:
>>>
>>>
>> Thanks for providing the preprocessed file.
>> I've reproduced and fixed the ICE in this version of the patch.
>> The problem was that I was taking the mode of x before the check
>> of whether a and b are MEMs, after which we would change x to an
>> address_mode reg,
>> thus confusing emit_move_insn.
>>
>> The fix is to take the mode of x and perform the can_conditionally_move_p check
>> after that transformation.
>>
>> Bootstrapped and tested on aarch64 and x86_64.
>> The preprocessed regex.i that Rainer provided now compiles successfully for me
>> on a sparc-sun-solaris2.10 stage-1 cross-compiler.
>>
>> Rainer, thanks for your help so far, could you please try out this patch?
> While bootstrap succeeds again, the testsuite regression in
> gcc.c-torture/execute/20071216-1.c reoccured.
Right, so I dug into the RTL dumps and I think this is a separate issue that's being exacerbated by my patch.
The code tries to if-convert a block which contains a compare instruction i.e. sets the CC register.
Now, bb_valid_for_noce_process_p should have caught this, and in particular insn_valid_noce_process_p
which should check that the instruction doesn't set the CC register. However, on SPARC the
cc_in_cond returns NULL! This is due to the canonicalize_comparison implementation that seems to
remove the CC register from the condition expression and returns something like:
(leu (reg/v:SI 109 [ b ])
     (const_int -4096 [0xfffffffffffff000])

Therefore the set_of (cc_in_cond (cond), insn) check doesn't get triggered because cc_in_cond returns NULL.
Regardless of how the branch condition got canonicalized, I think we still want to reject any insn in the block
that sets a condition code register, so this patch checks the destination of every set in the block for a MODE_CC
expression and cancels if-conversion if that's the case.

Oleg pointed me to the older PR 58517 affecting SH which seems similar and I think my previous ifcvt patch would expose
this problem more.

Anyway, with this patch the failing SPARC testcase gcc.c-torture/execute/20071216-1.c generates the same assembly
as before r227368 and bootstrap and test on aarch64 and x86_64 passes ok for me.

Rainer, could you try this patch on top of the previous patch? (https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00689.html)
The two together should fix all of PR 67456, 67464, 67465 and 67481.

Thanks,
Kyrill

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

     PR rtl-optimization/67481
     * ifcvt.c (contains_ccmode_rtx_p): New function.
     (insn_valid_noce_process_p): Use it.



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

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 9af3249..090a584 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1838,6 +1838,19 @@ noce_try_cmove (struct noce_if_info *if_info)
   return FALSE;
 }
 
+/* Return true if X contains a conditional code mode rtx.  */
+
+static bool
+contains_ccmode_rtx_p (rtx x)
+{
+  subrtx_iterator::array_type array;
+  FOR_EACH_SUBRTX (iter, array, x, ALL)
+    if (GET_MODE_CLASS (GET_MODE (*iter)) == MODE_CC)
+      return true;
+
+  return false;
+}
+
 /* Helper for bb_valid_for_noce_process_p.  Validate that
    the rtx insn INSN is a single set that does not set
    the conditional register CC and is in general valid for
@@ -1856,6 +1869,7 @@ insn_valid_noce_process_p (rtx_insn *insn, rtx cc)
   /* Currently support only simple single sets in test_bb.  */
   if (!sset
       || !noce_operand_ok (SET_DEST (sset))
+      || contains_ccmode_rtx_p (SET_DEST (sset))
       || !noce_operand_ok (SET_SRC (sset)))
     return false;
 

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Rainer Orth]

Hi Kyrill,

> On 11/09/15 09:51, Rainer Orth wrote:
>> Kyrill Tkachov <kyrylo.tkachov@arm.com> writes:
>>
>>> On 10/09/15 12:43, Rainer Orth wrote:
>>>> Hi Kyrill,
>>>>
>>>>> Rainer, could you please check that this patch still fixes the SPARC
>>>>> regressions?
>>>> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
>>>> stage2 libiberty/regex.c FAILs:
>>>>
>>>>
>>> Thanks for providing the preprocessed file.
>>> I've reproduced and fixed the ICE in this version of the patch.
>>> The problem was that I was taking the mode of x before the check
>>> of whether a and b are MEMs, after which we would change x to an
>>> address_mode reg,
>>> thus confusing emit_move_insn.
>>>
>>> The fix is to take the mode of x and perform the
>>> can_conditionally_move_p check
>>> after that transformation.
>>>
>>> Bootstrapped and tested on aarch64 and x86_64.
>>> The preprocessed regex.i that Rainer provided now compiles successfully
>>> for me
>>> on a sparc-sun-solaris2.10 stage-1 cross-compiler.
>>>
>>> Rainer, thanks for your help so far, could you please try out this patch?
>> While bootstrap succeeds again, the testsuite regression in
>> gcc.c-torture/execute/20071216-1.c reoccured.
> Right, so I dug into the RTL dumps and I think this is a separate issue
> that's being exacerbated by my patch.
> The code tries to if-convert a block which contains a compare instruction
> i.e. sets the CC register.
> Now, bb_valid_for_noce_process_p should have caught this, and in particular
> insn_valid_noce_process_p
> which should check that the instruction doesn't set the CC
> register. However, on SPARC the
> cc_in_cond returns NULL! This is due to the canonicalize_comparison
> implementation that seems to
> remove the CC register from the condition expression and returns something like:
> (leu (reg/v:SI 109 [ b ])
>     (const_int -4096 [0xfffffffffffff000])
>
> Therefore the set_of (cc_in_cond (cond), insn) check doesn't get triggered
> because cc_in_cond returns NULL.
> Regardless of how the branch condition got canonicalized, I think we still
> want to reject any insn in the block
> that sets a condition code register, so this patch checks the destination
> of every set in the block for a MODE_CC
> expression and cancels if-conversion if that's the case.
>
> Oleg pointed me to the older PR 58517 affecting SH which seems similar and
> I think my previous ifcvt patch would expose
> this problem more.
>
> Anyway, with this patch the failing SPARC testcase
> gcc.c-torture/execute/20071216-1.c generates the same assembly
> as before r227368 and bootstrap and test on aarch64 and x86_64 passes ok for me.
>
> Rainer, could you try this patch on top of the previous patch?
> (https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00689.html)
> The two together should fix all of PR 67456, 67464, 67465 and 67481.

sorry this took me so long: we've had a major switch failure and my
sparc machines are way slow.

Anyway, here's what I found: the two patches on top of each other do
bootstrap just fine and the gcc.c-torture/execute/20071216-1.c
regressions are gone.  However, it introduces a new one:

FAIL: gcc.dg/torture/stackalign/sibcall-1.c   -O1 -fpic execution test

It fails for both 32 and 64-bit.  The testcase SEGVs:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1 (LWP 1)]
0x00010bb0 in ix86_split_ashr (mode=1)
    at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:20
20                            : gen_x86_64_shrd) (0);
(gdb) where
#0  0x00010bb0 in ix86_split_ashr (mode=1)
    at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:20
#1  0x00010be4 in main ()
    at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:27
1: x/i $pc
=> 0x10bb0 <ix86_split_ashr+40>:        ld  [ %g1 ], %g1
(gdb) p/x $g1
$1 = 0x317f8

truss reveals:

    Incurred fault #6, FLTBOUNDS  %pc = 0x00010BB0
      siginfo: SIGSEGV SEGV_MAPERR addr=0x000317F8
    Received signal #11, SIGSEGV [default]
      siginfo: SIGSEGV SEGV_MAPERR addr=0x000317F8

with

#define FLTBOUNDS       6       /* Memory bounds (invalid address) */

and indeed that address isn't mapped according to

ro@apoc 58 > pmap 26536
26536:  /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe
00010000       8K r-x--  /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe
00020000       8K rwx--  /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe
FEE60000     696K r-x--  /lib/libm.so.2
FEF1C000      16K rwx--  /lib/libm.so.2
FF180000    1464K r-x--  /lib/libc.so.1
FF2FE000      48K rwx--  /lib/libc.so.1
FF350000      24K rwx--    [ anon ]
FF360000       8K rw---    [ anon ]
FF370000       8K rw---    [ anon ]
FF380000       8K rw---    [ anon ]
FF390000       8K rw---    [ anon ]
FF3A0000     248K r-x--  /lib/ld.so.1
FF3EE000      16K rwx--  /lib/ld.so.1
FFBFC000      16K rwx--    [ stack ]
 total      2576K

Something is totally amiss here.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

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

Hi Rainer,

On 17/09/15 12:33, Rainer Orth wrote:
> Hi Kyrill,
>
>> On 11/09/15 09:51, Rainer Orth wrote:
>>> Kyrill Tkachov <kyrylo.tkachov@arm.com> writes:
>>>
>>>> On 10/09/15 12:43, Rainer Orth wrote:
>>>>> Hi Kyrill,
>>>>>
>>>>>> Rainer, could you please check that this patch still fixes the SPARC
>>>>>> regressions?
>>>>> unfortunately, it breaks sparc-sun-solaris2.10 bootstrap: compiling
>>>>> stage2 libiberty/regex.c FAILs:
>>>>>
>>>>>
>>>> Thanks for providing the preprocessed file.
>>>> I've reproduced and fixed the ICE in this version of the patch.
>>>> The problem was that I was taking the mode of x before the check
>>>> of whether a and b are MEMs, after which we would change x to an
>>>> address_mode reg,
>>>> thus confusing emit_move_insn.
>>>>
>>>> The fix is to take the mode of x and perform the
>>>> can_conditionally_move_p check
>>>> after that transformation.
>>>>
>>>> Bootstrapped and tested on aarch64 and x86_64.
>>>> The preprocessed regex.i that Rainer provided now compiles successfully
>>>> for me
>>>> on a sparc-sun-solaris2.10 stage-1 cross-compiler.
>>>>
>>>> Rainer, thanks for your help so far, could you please try out this patch?
>>> While bootstrap succeeds again, the testsuite regression in
>>> gcc.c-torture/execute/20071216-1.c reoccured.
>> Right, so I dug into the RTL dumps and I think this is a separate issue
>> that's being exacerbated by my patch.
>> The code tries to if-convert a block which contains a compare instruction
>> i.e. sets the CC register.
>> Now, bb_valid_for_noce_process_p should have caught this, and in particular
>> insn_valid_noce_process_p
>> which should check that the instruction doesn't set the CC
>> register. However, on SPARC the
>> cc_in_cond returns NULL! This is due to the canonicalize_comparison
>> implementation that seems to
>> remove the CC register from the condition expression and returns something like:
>> (leu (reg/v:SI 109 [ b ])
>>      (const_int -4096 [0xfffffffffffff000])
>>
>> Therefore the set_of (cc_in_cond (cond), insn) check doesn't get triggered
>> because cc_in_cond returns NULL.
>> Regardless of how the branch condition got canonicalized, I think we still
>> want to reject any insn in the block
>> that sets a condition code register, so this patch checks the destination
>> of every set in the block for a MODE_CC
>> expression and cancels if-conversion if that's the case.
>>
>> Oleg pointed me to the older PR 58517 affecting SH which seems similar and
>> I think my previous ifcvt patch would expose
>> this problem more.
>>
>> Anyway, with this patch the failing SPARC testcase
>> gcc.c-torture/execute/20071216-1.c generates the same assembly
>> as before r227368 and bootstrap and test on aarch64 and x86_64 passes ok for me.
>>
>> Rainer, could you try this patch on top of the previous patch?
>> (https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00689.html)
>> The two together should fix all of PR 67456, 67464, 67465 and 67481.
> sorry this took me so long: we've had a major switch failure and my
> sparc machines are way slow.

No problem. You're doing me a huge favour by testing the iterations
of the patches.  Sorry for causing the regression in the first place.
The issues I'm finding are exposed due to the way the sparc backend
does some things, so my aarch64 and x86_64 testing is unlikely to catch them.

>
> Anyway, here's what I found: the two patches on top of each other do
> bootstrap just fine and the gcc.c-torture/execute/20071216-1.c
> regressions are gone.  However, it introduces a new one:
>
> FAIL: gcc.dg/torture/stackalign/sibcall-1.c   -O1 -fpic execution test
>
> It fails for both 32 and 64-bit.  The testcase SEGVs:

Indeed, I can see if-conversion triggering here and doing something funky with the
first patch that I posted (https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00689.html)

In this testcase we trigger the is_mem path through noce_try_cmove_arith
and we have the 'a' and 'b' having the form reg+symbol. When we try to move
them into a register with noce_emit_move_insn the resulting move is not recognised
(presumably sparc doesn't have any instructions/patterns to do this operation)
and the alternative tricks that noce_emit_move_insn tries to create the move
end up generating a bizzare sequence that involves loading from the memory at reg+symbol
expression and adding the base reg to it!

In any case, this patch doesn't try calling noce_emit_move_insn and instead generates a simple
SET expression, emits that and relies on end_ifcvt_sequence to call recog on it and cancel the
transformation if it's not a valid instruction. IMO this is the desired behaviour since the
move in question is supposed to be a simple move that would ideally be eliminated by the register
allocator later on if the dependencies work out. If it actually expands to more complex sequences
it's not going to be a win to if-convert anyway.

TLDR: This updated patch generates the same code for the sibcall-1.c testcase on sparc as before
the bad transformation and all the previous regressions are still fixed.

Bootstrapped and tested on aarch64 and x86_64.
Rainer, could you please try this patch in combination with the one I sent earlier at:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html


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

      PR rtl-optimization/67456
      PR rtl-optimization/67464
      PR rtl-optimization/67465
      * ifcvt.c (noce_try_cmove_arith): Bail out if cannot conditionally
      move in the mode of x.  Handle combination of complex and simple
      block pairs as well as the case when one is empty.

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

      * gcc.dg/pr67465.c: New test.

> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 1 (LWP 1)]
> 0x00010bb0 in ix86_split_ashr (mode=1)
>      at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:20
> 20                            : gen_x86_64_shrd) (0);
> (gdb) where
> #0  0x00010bb0 in ix86_split_ashr (mode=1)
>      at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:20
> #1  0x00010be4 in main ()
>      at /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/torture/stackalign/sibcall-1.c:27
> 1: x/i $pc
> => 0x10bb0 <ix86_split_ashr+40>:        ld  [ %g1 ], %g1
> (gdb) p/x $g1
> $1 = 0x317f8
>
> truss reveals:
>
>      Incurred fault #6, FLTBOUNDS  %pc = 0x00010BB0
>        siginfo: SIGSEGV SEGV_MAPERR addr=0x000317F8
>      Received signal #11, SIGSEGV [default]
>        siginfo: SIGSEGV SEGV_MAPERR addr=0x000317F8
>
> with
>
> #define FLTBOUNDS       6       /* Memory bounds (invalid address) */
>
> and indeed that address isn't mapped according to
>
> ro@apoc 58 > pmap 26536
> 26536:  /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe
> 00010000       8K r-x--  /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe
> 00020000       8K rwx--  /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/gcc/sibcall-1.exe
> FEE60000     696K r-x--  /lib/libm.so.2
> FEF1C000      16K rwx--  /lib/libm.so.2
> FF180000    1464K r-x--  /lib/libc.so.1
> FF2FE000      48K rwx--  /lib/libc.so.1
> FF350000      24K rwx--    [ anon ]
> FF360000       8K rw---    [ anon ]
> FF370000       8K rw---    [ anon ]
> FF380000       8K rw---    [ anon ]
> FF390000       8K rw---    [ anon ]
> FF3A0000     248K r-x--  /lib/ld.so.1
> FF3EE000      16K rwx--  /lib/ld.so.1
> FFBFC000      16K rwx--    [ stack ]
>   total      2576K
>
> Something is totally amiss here.
>
> 	Rainer
>


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

commit 9c327def49735ab179b68f2301fc4623ee45c974
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Mon Sep 7 14:58:01 2015 +0100

    [RTL-ifcvt] PR rtl-optimization/67465: Do not ifcvt complex blocks if the else block is empty

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 1e773d8..5b133f1 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2038,6 +2038,11 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
   insn_a = if_info->insn_a;
   insn_b = if_info->insn_b;
 
+  machine_mode x_mode = GET_MODE (x);
+
+  if (!can_conditionally_move_p (x_mode))
+    return FALSE;
+
   unsigned int then_cost;
   unsigned int else_cost;
   if (insn_a)
@@ -2074,13 +2079,38 @@ noce_try_cmove_arith (struct noce_if_info *if_info)
 	}
     }
 
-  if (!a_simple && then_bb && !b_simple && else_bb
+  if (then_bb && else_bb && !a_simple && !b_simple
       && (!bbs_ok_for_cmove_arith (then_bb, else_bb)
 	  || !bbs_ok_for_cmove_arith (else_bb, then_bb)))
     return FALSE;
 
   start_sequence ();
 
+  /* If one of the blocks is empty then the corresponding B or A value
+     came from the test block.  The non-empty complex block that we will
+     emit might clobber the register used by B or A, so move it to a pseudo
+     first.  */
+
+  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;
+    }
+
+  if (a_simple || !then_bb)
+    {
+      rtx tmp_a = gen_reg_rtx (x_mode);
+      emit_insn (gen_rtx_SET (tmp_a, a));
+      a = tmp_a;
+    }
+
   orig_a = a;
   orig_b = b;
 
diff --git a/gcc/testsuite/gcc.dg/pr67465.c b/gcc/testsuite/gcc.dg/pr67465.c
new file mode 100644
index 0000000..321fd38
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr67465.c
@@ -0,0 +1,53 @@
+/* { dg-do run } */
+/* { dg-options "-O3 -std=gnu99" } */
+
+int a, b, c, d, e, h;
+
+int
+fn1 (int p1)
+{
+  {
+    int g[2];
+    for (int i = 0; i < 1; i++)
+      g[i] = 0;
+    if (g[0] < c)
+      {
+	a = (unsigned) (1 ^ p1) % 2;
+	return 0;
+      }
+  }
+  return 0;
+}
+
+void
+fn2 ()
+{
+  for (h = 0; h < 1; h++)
+    {
+      for (int j = 0; j < 2; j++)
+	{
+	  for (b = 1; b; b = 0)
+	    a = 1;
+	  for (; b < 1; b++)
+	    ;
+	  if (e)
+	    continue;
+	  a = 2;
+	}
+      fn1 (h);
+      short k = -16;
+      d = k > a;
+    }
+}
+
+int
+main ()
+{
+  fn2 ();
+
+  if (a != 2)
+    __builtin_abort ();
+
+  return 0;
+}
+

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Rainer Orth]

Hi Kyrill,

> Bootstrapped and tested on aarch64 and x86_64.
> Rainer, could you please try this patch in combination with the one I sent
> earlier at:
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html

will do, however, Solaris/SPARC bootstrap is broken right now (PR
bootstrap/67622) and I'll have to hunt that down first.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Rainer Orth]

Hi Kyrill,

> Bootstrapped and tested on aarch64 and x86_64.
> Rainer, could you please try this patch in combination with the one I sent
> earlier at:
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html

it took me quite a bit, but I've now regtested those two patches: with
them both applied, the sparc-sun-solaris2.12 build succeeds and the two
gcc.c-torture/execute/20071216-1.c failures are gone.

So, from a SPARC POV the patches are good to go.

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

Hi Rainer,

On 25/09/15 11:57, Rainer Orth wrote:
> Hi Kyrill,
>
>> Bootstrapped and tested on aarch64 and x86_64.
>> Rainer, could you please try this patch in combination with the one I sent
>> earlier at:
>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
> it took me quite a bit, but I've now regtested those two patches: with
> them both applied, the sparc-sun-solaris2.12 build succeeds and the two
> gcc.c-torture/execute/20071216-1.c failures are gone.
>
> So, from a SPARC POV the patches are good to go.

Phew, thanks a lot!

So, in conclusion the patches I'd like approval for are:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01306.html
and
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html


Kyrill

>
> Thanks.
>          Rainer
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Jeff Law]

On 09/25/2015 05:06 AM, Kyrill Tkachov wrote:
> Hi Rainer,
>
> On 25/09/15 11:57, Rainer Orth wrote:
>> Hi Kyrill,
>>
>>> Bootstrapped and tested on aarch64 and x86_64.
>>> Rainer, could you please try this patch in combination with the one I
>>> sent
>>> earlier at:
>>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
>> it took me quite a bit, but I've now regtested those two patches: with
>> them both applied, the sparc-sun-solaris2.12 build succeeds and the two
>> gcc.c-torture/execute/20071216-1.c failures are gone.
>>
>> So, from a SPARC POV the patches are good to go.
>
> Phew, thanks a lot!
>
> So, in conclusion the patches I'd like approval for are:
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01306.html
> and
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
These are OK.  Thanks for taking the time to work with Rainer and sort 
out the sparc issues.  It's greatly appreciated.

Jeff

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[Kyrill Tkachov]

On 25/09/15 21:03, Jeff Law wrote:
> On 09/25/2015 05:06 AM, Kyrill Tkachov wrote:
>> Hi Rainer,
>>
>> On 25/09/15 11:57, Rainer Orth wrote:
>>> Hi Kyrill,
>>>
>>>> Bootstrapped and tested on aarch64 and x86_64.
>>>> Rainer, could you please try this patch in combination with the one I
>>>> sent
>>>> earlier at:
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
>>> it took me quite a bit, but I've now regtested those two patches: with
>>> them both applied, the sparc-sun-solaris2.12 build succeeds and the two
>>> gcc.c-torture/execute/20071216-1.c failures are gone.
>>>
>>> So, from a SPARC POV the patches are good to go.
>> Phew, thanks a lot!
>>
>> So, in conclusion the patches I'd like approval for are:
>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01306.html
>> and
>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
> These are OK.  Thanks for taking the time to work with Rainer and sort
> out the sparc issues.  It's greatly appreciated.

No problem, they were my regressions to fix after all, and it's easier to
fix now rather than in stage3/4.

I've committed them with r228194 and r228195.

Thanks,
Kyrill


> Jeff
>

Re: [PATCH][RTL-ifcvt] PR rtl-optimization/67465: Handle pairs of complex+simple blocks and empty blocks more gracefully

[H.J. Lu]

On Mon, Sep 28, 2015 at 1:26 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>
> On 25/09/15 21:03, Jeff Law wrote:
>>
>> On 09/25/2015 05:06 AM, Kyrill Tkachov wrote:
>>>
>>> Hi Rainer,
>>>
>>> On 25/09/15 11:57, Rainer Orth wrote:
>>>>
>>>> Hi Kyrill,
>>>>
>>>>> Bootstrapped and tested on aarch64 and x86_64.
>>>>> Rainer, could you please try this patch in combination with the one I
>>>>> sent
>>>>> earlier at:
>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
>>>>
>>>> it took me quite a bit, but I've now regtested those two patches: with
>>>> them both applied, the sparc-sun-solaris2.12 build succeeds and the two
>>>> gcc.c-torture/execute/20071216-1.c failures are gone.
>>>>
>>>> So, from a SPARC POV the patches are good to go.
>>>
>>> Phew, thanks a lot!
>>>
>>> So, in conclusion the patches I'd like approval for are:
>>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01306.html
>>> and
>>> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00815.html
>>
>> These are OK.  Thanks for taking the time to work with Rainer and sort
>> out the sparc issues.  It's greatly appreciated.
>
>
> No problem, they were my regressions to fix after all, and it's easier to
> fix now rather than in stage3/4.
>
> I've committed them with r228194 and r228195.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67749


-- 
H.J.