[PATCH] Fix improper combiner optimization of mixed mode vector/scalar expressions (PR 79365)

[PATCH] Fix improper combiner optimization of mixed mode vector/scalar expressions (PR 79365)

[Walter Lee]

Hi,

In looking at PR 79365 I found that the problem is actually in the
combiner.  The combiner sometimes applies scalar optimizations to
vector context where they are invalid.  i.e. (a > b) >> 1 can optimize
to 0 if ">" is a scalar op but not if it is a vector op.  The reason
this shows up on tile* and not other architectures is because tile*
uses the same register file for both scalars and vectors, so the
combiner sees these mixed mode expressions on tile* that would go
through memory on other architectures.

I have found two places where this improper optimization occurs.
Patch below.

Ok to commit?

Thanks,

Walter

diff --git a/gcc/combine.c b/gcc/combine.c
index 28133ff..8f2615e 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -7267,6 +7267,7 @@ expand_field_assignment (const_rtx x)
        else if (GET_CODE (SET_DEST (x)) == SUBREG
  	       /* We need SUBREGs to compute nonzero_bits properly.  */
  	       && nonzero_sign_valid
+	       && !VECTOR_MODE_P (GET_MODE (SET_DEST (x)))
  	       && (((GET_MODE_SIZE (GET_MODE (SET_DEST (x)))
  		     + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD)
  		   == ((GET_MODE_SIZE (GET_MODE (SUBREG_REG (SET_DEST (x))))
@@ -13030,6 +13031,7 @@ record_dead_and_set_regs_1 (rtx dest, const_rtx setter, void *data)
  	record_value_for_reg (dest, record_dead_insn, SET_SRC (setter));
        else if (GET_CODE (setter) == SET
  	       && GET_CODE (SET_DEST (setter)) == SUBREG
+	       && !VECTOR_MODE_P (GET_MODE (SET_DEST (setter)))
  	       && SUBREG_REG (SET_DEST (setter)) == dest
  	       && GET_MODE_PRECISION (GET_MODE (dest)) <= BITS_PER_WORD
  	       && subreg_lowpart_p (SET_DEST (setter)))
-- 
2.7.2

Re: [PATCH] Fix improper combiner optimization of mixed mode vector/scalar expressions (PR 79365)

[Walter Lee]

On 2/3/2017 3:03 PM, Walter Lee wrote:
> Hi,
>
> In looking at PR 79365 I found that the problem is actually in the
> combiner.  The combiner sometimes applies scalar optimizations to
> vector context where they are invalid.  i.e. (a > b) >> 1 can optimize
> to 0 if ">" is a scalar op but not if it is a vector op.  The reason
> this shows up on tile* and not other architectures is because tile*
> uses the same register file for both scalars and vectors, so the
> combiner sees these mixed mode expressions on tile* that would go
> through memory on other architectures.
>
> I have found two places where this improper optimization occurs.
> Patch below.
>
> Ok to commit?

I forgot to mention this fix has been validated on TILEPro/TILE-Gx.  I also sanity checked it against x86.

Thanks,

Walter

Re: [PATCH] Fix improper combiner optimization of mixed mode vector/scalar expressions (PR 79365)

[Segher Boessenkool]

Hi,

A couple of things.  Firstly, we are in stage 4 now, so unless this
is a regression it will have to wait for GCC 8.  I can carry the patch
or you can repost it when we are in stage 1.

Secondly, you forgot the changelog.

On Fri, Feb 03, 2017 at 03:03:06PM -0500, Walter Lee wrote:
> In looking at PR 79365 I found that the problem is actually in the
> combiner.  The combiner sometimes applies scalar optimizations to
> vector context where they are invalid.  i.e. (a > b) >> 1 can optimize
> to 0 if ">" is a scalar op but not if it is a vector op.  The reason
> this shows up on tile* and not other architectures is because tile*
> uses the same register file for both scalars and vectors, so the
> combiner sees these mixed mode expressions on tile* that would go
> through memory on other architectures.

There are other architectures that are similar, but perhaps none that
have their vector size equal to their most common integer size.

> I have found two places where this improper optimization occurs.

> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -7267,6 +7267,7 @@ expand_field_assignment (const_rtx x)
>        else if (GET_CODE (SET_DEST (x)) == SUBREG
>  	       /* We need SUBREGs to compute nonzero_bits properly.  */
>  	       && nonzero_sign_valid
> +	       && !VECTOR_MODE_P (GET_MODE (SET_DEST (x)))
>  	       && (((GET_MODE_SIZE (GET_MODE (SET_DEST (x)))
>  		     + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD)
>  		   == ((GET_MODE_SIZE (GET_MODE (SUBREG_REG (SET_DEST (x))))

Please use SCALAR_INT_MODE_P instead.

> @@ -13030,6 +13031,7 @@ record_dead_and_set_regs_1 (rtx dest, const_rtx 
> setter, void *data)
>  	record_value_for_reg (dest, record_dead_insn, SET_SRC (setter));
>        else if (GET_CODE (setter) == SET
>  	       && GET_CODE (SET_DEST (setter)) == SUBREG
> +	       && !VECTOR_MODE_P (GET_MODE (SET_DEST (setter)))
>  	       && SUBREG_REG (SET_DEST (setter)) == dest
>  	       && GET_MODE_PRECISION (GET_MODE (dest)) <= BITS_PER_WORD
>  	       && subreg_lowpart_p (SET_DEST (setter)))

What is this for?  It isn't triggered by the testcase in the PR.

You should add testcases to the testsuite, too.  In gcc.target is fine
if they are tile*-specific.

Thanks,


Segher

Re: [PATCH] Fix improper combiner optimization of mixed mode vector/scalar expressions (PR 79365)

[Walter Lee]

Thanks Segher.  I will address your comments and repost for GCC 8.

> What is this for?  It isn't triggered by the testcase in the PR.

It was triggered by our strcmp code, but I didn't have a simple test case for it.  I will try to generate one.

Walter

    

Re: [PATCH] Fix improper combiner optimization of mixed mode vector/scalar expressions (PR 79365)

[Jeff Law]

On 02/03/2017 01:03 PM, Walter Lee wrote:
> Hi,
> 
> In looking at PR 79365 I found that the problem is actually in the
> combiner.  The combiner sometimes applies scalar optimizations to
> vector context where they are invalid.  i.e. (a > b) >> 1 can optimize
> to 0 if ">" is a scalar op but not if it is a vector op.  The reason
> this shows up on tile* and not other architectures is because tile*
> uses the same register file for both scalars and vectors, so the
> combiner sees these mixed mode expressions on tile* that would go
> through memory on other architectures.
> 
> I have found two places where this improper optimization occurs.
> Patch below.
> 
> Ok to commit?
Walter, now that the trunk is open, it's a good time to address this issue.
> 
> Thanks,
> 
> Walter
> 
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 28133ff..8f2615e 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -7267,6 +7267,7 @@ expand_field_assignment (const_rtx x)
>         else if (GET_CODE (SET_DEST (x)) == SUBREG
>              /* We need SUBREGs to compute nonzero_bits properly.  */
>              && nonzero_sign_valid
> +           && !VECTOR_MODE_P (GET_MODE (SET_DEST (x)))
>              && (((GET_MODE_SIZE (GET_MODE (SET_DEST (x)))
>                + (UNITS_PER_WORD - 1)) / UNITS_PER_WORD)
>              == ((GET_MODE_SIZE (GET_MODE (SUBREG_REG (SET_DEST (x))))
> @@ -13030,6 +13031,7 @@ record_dead_and_set_regs_1 (rtx dest, const_rtx 
> setter, void *data)
>       record_value_for_reg (dest, record_dead_insn, SET_SRC (setter));
>         else if (GET_CODE (setter) == SET
>              && GET_CODE (SET_DEST (setter)) == SUBREG
> +           && !VECTOR_MODE_P (GET_MODE (SET_DEST (setter)))
>              && SUBREG_REG (SET_DEST (setter)) == dest
>              && GET_MODE_PRECISION (GET_MODE (dest)) <= BITS_PER_WORD
>              && subreg_lowpart_p (SET_DEST (setter)))
Segher's comments still apply.  Specifically:

Use SCALAR_INT_MODE_P rather than !VECTOR_MODE_P and please add 
testcases to the testsuite.  It is fine to put the testcases in 
gcc.target if they are tile specific.

Please repost after making those changes.

Thanks,
Jeff