Fix PR tree-optimization/58137

On Mon, Sep 2, 2013 at 8:53 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On Fri, 30 Aug 2013 12:34:51 Richard Biener wrote:
>> On Tue, Aug 20, 2013 at 1:01 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> This patch fixes a bug in the vectorized pointer arithmetic in the forwprop
>>> optimization pass.
>>>
>>> Although it seems to be impossible to create a vector of pointers with the
>>> __attribute__((vector_size(x))) syntax, the vector loop optimization together
>>> with the loop unrolling can create code which adds a vector of ptroff_t
>>> repeatedly to a vector of pointers.
>>>
>>> The code in tree-ssa-forwprop.c handles program transformations of the
>>> form (PTR +- CST1) +- CST2 => PTR +- (CST1+-CST2) where PTR is
>>> a vector of pointers and CST1 and CST2 are vectors of ptroff_t, without the
>>> fix the result type of CST1+-CST2 was vector of pointer, which causes the
>>> ICE in tree-cfg.c, which sees an undefined pointer + pointer operation.
>>>
>>> Additionally the check in tree-cfg.c allows expressions of the form CST - PTR,
>>> which is clearly wrong. That should be fixed too.
>>>
>>> The patch was bootstrapped and regression tested on i686-pc-linux-gnu.
>>
>> It seems to me that the forwprop code does not handle the fact that we
>> are permissive as to using PLUS_EXPR instead of POINTER_PLUS_EXPR
>> for vector pointer - offset additions. So instead of doing this dance the
>> better (and more easily backportable) fix is to simply disable the transform
>> on pointer valued PLUS_EXPR. Like with
>>
>> if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt))))
>> return false;
>>
>> at the beginning of the function.
>>
>
> the condition would have to be:
>
>   if (TREE_CODE (TREE_TYPE (gimple_assign_lhs (stmt))) == VECTOR_TYPE
>       && POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (gimple_assign_lhs (stmt)))))
>     return false;
>
> I tried this, and it fixes the ICE. However the generated code was still vectorized but
> much less efficient and used more registers.
>
> Fortunately there will be no need to backport this, since this bug does not happen in
> gcc 4.8.2 I checked that. I believe it was introduced with the checkin r200059 by
> Marc Glisse where associate_plusminus was enhanced to handle vector values.
> Before that only TREE_CODE (rhs2) == INTEGER_CST was handled.
>
> Frankly I would prefer the initial version of the patch, because the code is more
> efficient this way. The vector data is folded correctly, only the data type was wrong
> and triggered the ICE in tree-cfg.c.
>
> Please advise.

I'd rather go with the simple fix as the issue in forwprop is at least
latent.  We can
improve on the code-gen as followup where I believe handling of
POINTER_PLUS_EXPR
would need to be added (that we avoid POINTER_PLUS_EXPR for vectors is a bug).
That can be done in a way to cover the vector case properly.  Or
finally properly
use POINTER_PLUS_EXPR for vectors or make the vectorizer not use pointer
types but a corresponding unsigned integer type for them (that would also fix
the original bug of course).  Like with (untested)


actually that would be my preference here ...

Thanks,
Richard.

> Thanks
> Bernd.
>
>> The real fix is of course to make vector pointer operations properly
>> use POINTER_PLUS_EXPR ...
>>
>> Richard.
>>
>>
>>
>>> Regards
>>> Bernd Edlinger

On Tue, 3 Sep 2013 10:38:33, Richard Biener wrote:
> I'd rather go with the simple fix as the issue in forwprop is at least
> latent. We can
> improve on the code-gen as followup where I believe handling of
> POINTER_PLUS_EXPR
> would need to be added (that we avoid POINTER_PLUS_EXPR for vectors is a bug).
> That can be done in a way to cover the vector case properly. Or
> finally properly
> use POINTER_PLUS_EXPR for vectors or make the vectorizer not use pointer
> types but a corresponding unsigned integer type for them (that would also fix
> the original bug of course). Like with (untested)
>
> Index: gcc/tree-vect-stmts.c
> ===================================================================
> --- gcc/tree-vect-stmts.c (revision 202196)
> +++ gcc/tree-vect-stmts.c (working copy)
> @@ -6179,8 +6179,7 @@ get_vectype_for_scalar_type_and_size (tr
> corresponding to that mode. The theory is that any use that
> would cause problems with this will disable vectorization anyway. */
> else if (!SCALAR_FLOAT_TYPE_P (scalar_type)
> - && !INTEGRAL_TYPE_P (scalar_type)
> - && !POINTER_TYPE_P (scalar_type))
> + && !INTEGRAL_TYPE_P (scalar_type))
> scalar_type = lang_hooks.types.type_for_mode (inner_mode, 1);
>
> /* We can't build a vector type of elements with alignment bigger than
>
> actually that would be my preference here ...

this would cause an ICE in test case 20000629-1.c...

So removing the pointer of vectors is not an option.

>>> The real fix is of course to make vector pointer operations properly
>>> use POINTER_PLUS_EXPR ...

Okay I can do what you want, and use POINTER_PLUS_EXPR for
vectors of pointers, and do the constant folding in assocate_pointerplus.
This way we get exactly the same code as before. It may be even possible
that this constant folding can improve something with scalars.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.

OK for trunk?

Bernd.
2013-09-05  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR tree-optimization/58137
	Use POINTER_PLUS_EXPR for vector of pointer additions.
	* tree-cfg.c (verify_gimple_assign_binary)
	<PLUS_EXPR, MINUS_EXPR>: Remove vector of pointer handling.
	<POINTER_PLUS_EXPR>: Add vector of pointer type check.
	* tree.c (build2_stat) <POINTER_PLUS_EXPR>: Allow vector of pointer.
	* tree-vect-stmts.c (vectorizable_operation): Do not replace
	POINTER_PLUS_EXPR with PLUS_EXPR.
	* tree-vect-loop.c (get_initial_def_for_induction): Use
	POINTER_PLUS_EXPR for vector of pointer.

testsuite:

	* gcc.target/i386/pr58137.c: New test.

On Thu, Sep 5, 2013 at 11:43 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On Tue, 3 Sep 2013 10:38:33, Richard Biener wrote:
>> I'd rather go with the simple fix as the issue in forwprop is at least
>> latent. We can
>> improve on the code-gen as followup where I believe handling of
>> POINTER_PLUS_EXPR
>> would need to be added (that we avoid POINTER_PLUS_EXPR for vectors is a bug).
>> That can be done in a way to cover the vector case properly. Or
>> finally properly
>> use POINTER_PLUS_EXPR for vectors or make the vectorizer not use pointer
>> types but a corresponding unsigned integer type for them (that would also fix
>> the original bug of course). Like with (untested)
>>
>> Index: gcc/tree-vect-stmts.c
>> ===================================================================
>> --- gcc/tree-vect-stmts.c (revision 202196)
>> +++ gcc/tree-vect-stmts.c (working copy)
>> @@ -6179,8 +6179,7 @@ get_vectype_for_scalar_type_and_size (tr
>> corresponding to that mode. The theory is that any use that
>> would cause problems with this will disable vectorization anyway. */
>> else if (!SCALAR_FLOAT_TYPE_P (scalar_type)
>> - && !INTEGRAL_TYPE_P (scalar_type)
>> - && !POINTER_TYPE_P (scalar_type))
>> + && !INTEGRAL_TYPE_P (scalar_type))
>> scalar_type = lang_hooks.types.type_for_mode (inner_mode, 1);
>>
>> /* We can't build a vector type of elements with alignment bigger than
>>
>> actually that would be my preference here ...
>
> this would cause an ICE in test case 20000629-1.c...

Well, that's easily fixable.

> So removing the pointer of vectors is not an option.

Let me nevertheless try this.

Richard.

>>>> The real fix is of course to make vector pointer operations properly
>>>> use POINTER_PLUS_EXPR ...
>
> Okay I can do what you want, and use POINTER_PLUS_EXPR for
> vectors of pointers, and do the constant folding in assocate_pointerplus.
> This way we get exactly the same code as before. It may be even possible
> that this constant folding can improve something with scalars.
>
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.
>
> OK for trunk?
>
> Bernd.

On Thu, 5 Sep 2013 12:25:08, Richard Biener wrote:
> On Thu, Sep 5, 2013 at 11:43 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>>
>> this would cause an ICE in test case 20000629-1.c...
>
> Well, that's easily fixable.
>
>> So removing the pointer of vectors is not an option.
>
> Let me nevertheless try this.
>
> Richard.

Very well, then you continue on this PR and I'll come back
later with a follow up patch.

Bernd.