diff mbox

[5.1,rs6000] Fix PR65787

Message ID 1429288364.20720.17.camel@gnopaine
State New
Headers show

Commit Message

Bill Schmidt April 17, 2015, 4:32 p.m. UTC 
Hi,

On Fri, 2015-04-17 at 10:02 -0500, Bill Schmidt wrote:
> On Fri, 2015-04-17 at 16:49 +0200, Jakub Jelinek wrote:
> > You have actually mailed the original patch again, not the revised one.
> 
> > That said, PARALLEL seems to be already handled by rtx_is_swappable_p,
> > so if it isn't handled correctly, perhaps there is a bug in that function.
> > 

Quite right.  I've fixed this as you suggested in the attached patch.
Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
Is this version ok?

Sorry for apparently not knowing my own code as well as I should... :/

Thanks,
Bill


2015-04-17  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR target/65787
	* config/rs6000/rs6000.c (rtx_is_swappable_p): Remove previous
	fix; ensure that a subsequent SH_NONE operand does not overwrite
	an existing *special value.

Comments

Jakub Jelinek April 17, 2015, 4:39 p.m. UTC | #1 
On Fri, Apr 17, 2015 at 11:32:44AM -0500, Bill Schmidt wrote:
> 2015-04-17  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	PR target/65787
> 	* config/rs6000/rs6000.c (rtx_is_swappable_p): Remove previous
> 	fix; ensure that a subsequent SH_NONE operand does not overwrite
> 	an existing *special value.
> 
> 
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 222182)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -34204,17 +34204,6 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
>        else
>  	return 0;
>  
> -    case PARALLEL:
> -      /* A vec_extract operation may be wrapped in a PARALLEL with a
> -	 clobber, so account for that possibility.  */
> -      if (XVECLEN (op, 0) != 2)
> -	return 0;
> -
> -      if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
> -	return 0;
> -
> -      return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
> -
>      case UNSPEC:
>        {
>  	/* Various operations are unsafe for this optimization, at least
> @@ -34308,6 +34297,8 @@ rtx_is_swappable_p (rtx op, unsigned int *special)
>  	{
>  	  unsigned int special_op = SH_NONE;
>  	  ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
> +	  if (special_op == SH_NONE)
> +	    continue;
>  	  /* Ensure we never have two kinds of special handling
>  	     for the same insn.  */
>  	  if (*special != SH_NONE && special_op != SH_NONE

The " && special_op != SH_NONE" test from the second if can go then,
because it is never true.  And I'd really think that you shouldn't change
just the fmt[i] == 'E' handling, but also the fmt[i] == 'e' || fmt[i] == 'u'
handling a few lines earlier (both the added
"if (special_op == SH_NONE) continue;" there and
removal of " && special_op != SH_NONE".

	Jakub
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 222182)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -34204,17 +34204,6 @@  rtx_is_swappable_p (rtx op, unsigned int *special)
       else
 	return 0;
 
-    case PARALLEL:
-      /* A vec_extract operation may be wrapped in a PARALLEL with a
-	 clobber, so account for that possibility.  */
-      if (XVECLEN (op, 0) != 2)
-	return 0;
-
-      if (GET_CODE (XVECEXP (op, 0, 1)) != CLOBBER)
-	return 0;
-
-      return rtx_is_swappable_p (XVECEXP (op, 0, 0), special);
-
     case UNSPEC:
       {
 	/* Various operations are unsafe for this optimization, at least
@@ -34308,6 +34297,8 @@  rtx_is_swappable_p (rtx op, unsigned int *special)
 	{
 	  unsigned int special_op = SH_NONE;
 	  ok &= rtx_is_swappable_p (XVECEXP (op, i, j), &special_op);
+	  if (special_op == SH_NONE)
+	    continue;
 	  /* Ensure we never have two kinds of special handling
 	     for the same insn.  */
 	  if (*special != SH_NONE && special_op != SH_NONE