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

[Segher Boessenkool <segher@kernel.crashing.org>]

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