[patch] Fix ICE on ACATS test for Aarch64 at -O

[patch] Fix ICE on ACATS test for Aarch64 at -O

[Eric Botcazou]

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

Hi,

for the attached reduced testcase, the ICE is:

eric@polaris:~/gnat/bugs/P901-028> ~/build/gcc/aarch64-linux/gcc/gnat1 -quiet 
p.adb -O -I ~/build/gcc/aarch64-linux/gcc/ada/rts
+===========================GNAT BUG DETECTED==============================+
| 7.0.0 20160914 (experimental) [trunk revision 240142] (aarch64-linux) GCC 
error:|
| in expand_shift_1, at expmed.c:2451                                      |
| Error detected around p.adb:21:29

#0  internal_error (gmsgid=0x22327b7 "in %s, at %s:%d")
    at /home/eric/svn/gcc/gcc/diagnostic.c:1347
#1  0x0000000001e2373b in fancy_abort (
    file=0x1f965f8 "/home/eric/svn/gcc/gcc/expmed.c", line=2451, 
    function=0x1f96ce7 <expand_shift_1(tree_code, machine_mode, rtx_def*, 
rtx_def*, rtx_def*, int)::__FUNCTION__> "expand_shift_1")
    at /home/eric/svn/gcc/gcc/diagnostic.c:1415
#2  0x0000000000eb435c in expand_shift_1 (code=RSHIFT_EXPR, mode=OImode, 
    shifted=0x7ffff68e02e8, amount=0x7ffff68bcef0, target=0x0, unsignedp=1)
    at /home/eric/svn/gcc/gcc/expmed.c:2451
#3  0x0000000000eb43bd in expand_shift (code=RSHIFT_EXPR, mode=OImode, 
    shifted=0x7ffff68e02e8, amount=255, target=0x0, unsignedp=1)
    at /home/eric/svn/gcc/gcc/expmed.c:2467
#4  0x0000000000ebefe3 in emit_store_flag (target=0x7ffff68de780, code=NE, 
    op0=0x7ffff68de798, op1=0x7ffff6c3d400, mode=OImode, unsignedp=1, 
    normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5826
#5  0x0000000000ebe979 in emit_store_flag (target=0x7ffff68de780, code=NE, 
    op0=0x7ffff68dc138, op1=0x7ffff68dc030, mode=OImode, unsignedp=1, 
    normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5670
#6  0x0000000000ebf0ab in emit_store_flag_force (target=0x7ffff68de780, 
    code=NE, op0=0x7ffff68dc138, op1=0x7ffff68dc030, mode=OImode, unsignedp=1, 
    normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5860
#7  0x0000000000ef0aba in do_store_flag (ops=0x7fffffffd4d0, 
    target=0x7ffff68de780, mode=QImode) at /home/eric/svn/gcc/gcc/expr.c:11408
#8  0x0000000000ee6873 in expand_expr_real_2 (ops=0x7fffffffd4d0, target=0x0, 
    tmode=QImode, modifier=EXPAND_NORMAL) at 
/home/eric/svn/gcc/gcc/expr.c:9196

It's an attempt at generating a store-flag sequence with OImode and it fails 
because there are no shift operations supported in that mode.  It turns out 
that emit_store_flag_force knows how to fall back on a branchy sequence in 
that case so the attached patch simply removes the assertion.

Tested on x86-64/Linux, OK for the mainline?


2016-09-16  Eric Botcazou  <ebotcazou@adacore.com>

	* expmed.c (expand_shift_1): Remove assertion and adjust comment.
	(expand_shift): Adjust comment.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1194 bytes --]

Index: expmed.c
===================================================================
--- expmed.c	(revision 240142)
+++ expmed.c	(working copy)
@@ -2247,7 +2247,7 @@ expand_dec (rtx target, rtx dec)
    and AMOUNT the rtx for the amount to shift by.
    Store the result in the rtx TARGET, if that is convenient.
    If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
-   Return the rtx for where the value is.  */
+   Return the rtx for where the value is or 0 if that cannot be done.  */
 
 static rtx
 expand_shift_1 (enum tree_code code, machine_mode mode, rtx shifted,
@@ -2448,7 +2448,6 @@ expand_shift_1 (enum tree_code code, mac
 	 define_expand for lshrsi3 was added to vax.md.  */
     }
 
-  gcc_assert (temp);
   return temp;
 }
 
@@ -2457,7 +2456,7 @@ expand_shift_1 (enum tree_code code, mac
    and AMOUNT the amount to shift by.
    Store the result in the rtx TARGET, if that is convenient.
    If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
-   Return the rtx for where the value is.  */
+   Return the rtx for where the value is, or 0 if that cannot be done.  */
 
 rtx
 expand_shift (enum tree_code code, machine_mode mode, rtx shifted,

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Jeff Law]

On 09/16/2016 04:05 PM, Eric Botcazou wrote:
> Hi,
>
> for the attached reduced testcase, the ICE is:
>
> eric@polaris:~/gnat/bugs/P901-028> ~/build/gcc/aarch64-linux/gcc/gnat1 -quiet
> p.adb -O -I ~/build/gcc/aarch64-linux/gcc/ada/rts
> +===========================GNAT BUG DETECTED==============================+
> | 7.0.0 20160914 (experimental) [trunk revision 240142] (aarch64-linux) GCC
> error:|
> | in expand_shift_1, at expmed.c:2451                                      |
> | Error detected around p.adb:21:29
>
> #0  internal_error (gmsgid=0x22327b7 "in %s, at %s:%d")
>     at /home/eric/svn/gcc/gcc/diagnostic.c:1347
> #1  0x0000000001e2373b in fancy_abort (
>     file=0x1f965f8 "/home/eric/svn/gcc/gcc/expmed.c", line=2451,
>     function=0x1f96ce7 <expand_shift_1(tree_code, machine_mode, rtx_def*,
> rtx_def*, rtx_def*, int)::__FUNCTION__> "expand_shift_1")
>     at /home/eric/svn/gcc/gcc/diagnostic.c:1415
> #2  0x0000000000eb435c in expand_shift_1 (code=RSHIFT_EXPR, mode=OImode,
>     shifted=0x7ffff68e02e8, amount=0x7ffff68bcef0, target=0x0, unsignedp=1)
>     at /home/eric/svn/gcc/gcc/expmed.c:2451
> #3  0x0000000000eb43bd in expand_shift (code=RSHIFT_EXPR, mode=OImode,
>     shifted=0x7ffff68e02e8, amount=255, target=0x0, unsignedp=1)
>     at /home/eric/svn/gcc/gcc/expmed.c:2467
> #4  0x0000000000ebefe3 in emit_store_flag (target=0x7ffff68de780, code=NE,
>     op0=0x7ffff68de798, op1=0x7ffff6c3d400, mode=OImode, unsignedp=1,
>     normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5826
> #5  0x0000000000ebe979 in emit_store_flag (target=0x7ffff68de780, code=NE,
>     op0=0x7ffff68dc138, op1=0x7ffff68dc030, mode=OImode, unsignedp=1,
>     normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5670
> #6  0x0000000000ebf0ab in emit_store_flag_force (target=0x7ffff68de780,
>     code=NE, op0=0x7ffff68dc138, op1=0x7ffff68dc030, mode=OImode, unsignedp=1,
>     normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5860
> #7  0x0000000000ef0aba in do_store_flag (ops=0x7fffffffd4d0,
>     target=0x7ffff68de780, mode=QImode) at /home/eric/svn/gcc/gcc/expr.c:11408
> #8  0x0000000000ee6873 in expand_expr_real_2 (ops=0x7fffffffd4d0, target=0x0,
>     tmode=QImode, modifier=EXPAND_NORMAL) at
> /home/eric/svn/gcc/gcc/expr.c:9196
>
> It's an attempt at generating a store-flag sequence with OImode and it fails
> because there are no shift operations supported in that mode.  It turns out
> that emit_store_flag_force knows how to fall back on a branchy sequence in
> that case so the attached patch simply removes the assertion.
>
> Tested on x86-64/Linux, OK for the mainline?
>
>
> 2016-09-16  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* expmed.c (expand_shift_1): Remove assertion and adjust comment.
> 	(expand_shift): Adjust comment.
Is this really safe.  If I look at where we call 
expand_shift/expand_shift_1 I get real worried that we could end up 
using that NULL value in a context where it's not expected.

So while emit_store_flag_force knows what to do upon NULL return, I'm 
not sure the other users of expand_shift/expand_shift_1 do.  For example 
in expand_bit_field_1:

       target = expand_shift (LSHIFT_EXPR, mode, target,
                              GET_MODE_BITSIZE (mode) - bitsize, 
NULL_RTX, 0);
       return expand_shift (RSHIFT_EXPR, mode, target,
                            GET_MODE_BITSIZE (mode) - bitsize, NULL_RTX, 0);

If the first call returned NULL, but the 2nd didn't, then we end up with 
a NULL argument in the RSHIFT_EXPR node we create.

or extract_fixed_bit_field_1:

      if (bitnum)
         {
           /* If the field does not already start at the lsb,
              shift it so it does.  */
           /* Maybe propagate the target for the shift.  */
           rtx subtarget = (target != 0 && REG_P (target) ? target : 0);
           if (tmode != mode)
             subtarget = 0;
           op0 = expand_shift (RSHIFT_EXPR, mode, op0, bitnum, 
subtarget, 1);
         }
       /* Convert the value to the desired mode.  */
       if (mode != tmode)
         op0 = convert_to_mode (tmode, op0, 1);

Can we end up with a NULL op0 which we then pass to convert_to_mode?

Is there any way to address this problem in emit_store_flag_force?

jeff

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Eric Botcazou]

> Is this really safe.  If I look at where we call
> expand_shift/expand_shift_1 I get real worried that we could end up
> using that NULL value in a context where it's not expected.
>
> So while emit_store_flag_force knows what to do upon NULL return, I'm
> not sure the other users of expand_shift/expand_shift_1 do.

Yes, that's probably risky, and there are a lot of calls to them.

> Is there any way to address this problem in emit_store_flag_force?

Now that we're in C++, we could add an additional argument to expand_shift_1 
with a default value and call the function directly from emit_store_flag with 
the non-default value, thus instructing it to return 0 on failure instead of 
aborting.  emit_store_flag already returns 0 on failure so the result would be 
naturally propagated up to emit_store_flag_force.

-- 
Eric Botcazou

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Eric Botcazou]

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

> So while emit_store_flag_force knows what to do upon NULL return, I'm
> not sure the other users of expand_shift/expand_shift_1 do.  

Revised patch attached, tested on x86-64/Linux, OK for the mainline?


2016-09-26  Eric Botcazou  <ebotcazou@adacore.com>

	* expmed.c (expand_shift_1): Add MAY_FAIL parameter and do not assert
	that the result is non-zero if it is true.
	(maybe_expand_shift): New wrapper around expand_shift_1.
	(emit_store_flag): Call maybe_expand_shift in lieu of expand_shift.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 2599 bytes --]

Index: expmed.c
===================================================================
--- expmed.c	(revision 240311)
+++ expmed.c	(working copy)
@@ -2247,11 +2247,13 @@ expand_dec (rtx target, rtx dec)
    and AMOUNT the rtx for the amount to shift by.
    Store the result in the rtx TARGET, if that is convenient.
    If UNSIGNEDP is nonzero, do a logical shift; otherwise, arithmetic.
-   Return the rtx for where the value is.  */
+   Return the rtx for where the value is.
+   If that cannot be done, abort the compilation unless MAY_FAIL is true,
+   in which case 0 is returned.  */
 
 static rtx
 expand_shift_1 (enum tree_code code, machine_mode mode, rtx shifted,
-		rtx amount, rtx target, int unsignedp)
+		rtx amount, rtx target, int unsignedp, bool may_fail = false)
 {
   rtx op1, temp = 0;
   int left = (code == LSHIFT_EXPR || code == LROTATE_EXPR);
@@ -2448,7 +2450,7 @@ expand_shift_1 (enum tree_code code, mac
 	 define_expand for lshrsi3 was added to vax.md.  */
     }
 
-  gcc_assert (temp);
+  gcc_assert (temp != NULL_RTX || may_fail);
   return temp;
 }
 
@@ -2467,6 +2469,16 @@ expand_shift (enum tree_code code, machi
 			 shifted, GEN_INT (amount), target, unsignedp);
 }
 
+/* Likewise, but return 0 if that cannot be done.  */
+
+static rtx
+maybe_expand_shift (enum tree_code code, machine_mode mode, rtx shifted,
+		    int amount, rtx target, int unsignedp)
+{
+  return expand_shift_1 (code, mode,
+			 shifted, GEN_INT (amount), target, unsignedp, true);
+}
+
 /* Output a shift instruction for expression code CODE,
    with SHIFTED being the rtx for the value to shift,
    and AMOUNT the tree for the amount to shift by.
@@ -5753,11 +5765,12 @@ emit_store_flag (rtx target, enum rtx_co
       if (rtx_equal_p (subtarget, op0))
 	subtarget = 0;
 
-      tem = expand_shift (RSHIFT_EXPR, mode, op0,
-			  GET_MODE_BITSIZE (mode) - 1,
-			  subtarget, 0);
-      tem = expand_binop (mode, sub_optab, tem, op0, subtarget, 0,
-			  OPTAB_WIDEN);
+      tem = maybe_expand_shift (RSHIFT_EXPR, mode, op0,
+				GET_MODE_BITSIZE (mode) - 1,
+				subtarget, 0);
+      if (tem)
+	tem = expand_binop (mode, sub_optab, tem, op0, subtarget, 0,
+			    OPTAB_WIDEN);
     }
 
   if (code == EQ || code == NE)
@@ -5819,9 +5832,9 @@ emit_store_flag (rtx target, enum rtx_co
     }
 
   if (tem && normalizep)
-    tem = expand_shift (RSHIFT_EXPR, mode, tem,
-			GET_MODE_BITSIZE (mode) - 1,
-			subtarget, normalizep == 1);
+    tem = maybe_expand_shift (RSHIFT_EXPR, mode, tem,
+			      GET_MODE_BITSIZE (mode) - 1,
+			      subtarget, normalizep == 1);
 
   if (tem)
     {

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Eric Botcazou]

Ping: https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01781.html

> 2016-09-26  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* expmed.c (expand_shift_1): Add MAY_FAIL parameter and do not assert
> 	that the result is non-zero if it is true.
> 	(maybe_expand_shift): New wrapper around expand_shift_1.
> 	(emit_store_flag): Call maybe_expand_shift in lieu of expand_shift.

-- 
Eric Botcazou

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Eric Botcazou]

Ping^2: https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01781.html

> 2016-09-26  Eric Botcazou  <ebotcazou@adacore.com>
> 
>       * expmed.c (expand_shift_1): Add MAY_FAIL parameter and do not assert
>       that the result is non-zero if it is true.
>       (maybe_expand_shift): New wrapper around expand_shift_1.
>       (emit_store_flag): Call maybe_expand_shift in lieu of expand_shift.

-- 
Eric Botcazou

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Eric Botcazou]

> Revised patch attached, tested on x86-64/Linux, OK for the mainline?

Also tested on Aarch64/Linux, where it cleans up the ACATS testsuite:

--- mail-report.log.0   2016-10-11 04:01:06.020972999 -0700
+++ mail-report.log     2016-10-11 14:27:42.110972999 -0700
@@ -2,12 +2,10 @@
 LAST_UPDATED: Obtained from SVN: trunk revision 240904
 
                === acats tests ===
-FAIL:  c34005o
-FAIL:  c34007i
 
                === acats Summary ===
-# of expected passes           2318
-# of unexpected failures       2
+# of expected passes           2320
+# of unexpected failures       0
 Native configuration is aarch64-unknown-linux-gnu

-- 
Eric Botcazou

Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Jeff Law]

On 10/03/2016 09:45 AM, Eric Botcazou wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01781.html
>
>> 2016-09-26  Eric Botcazou  <ebotcazou@adacore.com>
>>
>> 	* expmed.c (expand_shift_1): Add MAY_FAIL parameter and do not assert
>> 	that the result is non-zero if it is true.
>> 	(maybe_expand_shift): New wrapper around expand_shift_1.
>> 	(emit_store_flag): Call maybe_expand_shift in lieu of expand_shift.
>
OK.  Sorry for the delay.

jeff