Re: [PATCH] Fix PR tree-optimization/58137

[Richard Biener <richard dot guenther at gmail dot com>]

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)

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 ...

Thanks,
Richard.

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