[patch] Fix segfault in vectorizer

[patch] Fix segfault in vectorizer

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]

Hi,

it's a regression present on the mainline and 6 branch: for the attached Ada 
testcase, optab_for_tree_code segfaults because the function is invoked on a 
NULL_TREE vectype_out from the vectorizer (vectorizable_reduction):

  if (STMT_VINFO_VEC_REDUCTION_TYPE (stmt_info) == TREE_CODE_REDUCTION
      || STMT_VINFO_VEC_REDUCTION_TYPE (stmt_info)
		== INTEGER_INDUC_COND_REDUCTION)
    {
      if (reduction_code_for_scalar_code (orig_code, &epilog_reduc_code))
	{
	  reduc_optab = optab_for_tree_code (epilog_reduc_code, vectype_out,
                                         optab_default);

Naive attempts at working around the NULL_TREE result in bogus vectorization 
and GIMPLE verification failure so it seems clear that vectype_out ought not 
to be NULL_TREE here.

The problems stems from vect_determine_vectorization_factor, namely:

	      /* Bool ops don't participate in vectorization factor
		 computation.  For comparison use compared types to
		 compute a factor.  */
	      if (TREE_CODE (scalar_type) == BOOLEAN_TYPE
		  && is_gimple_assign (stmt)
		  && gimple_assign_rhs_code (stmt) != COND_EXPR)
		{
		  if (STMT_VINFO_RELEVANT_P (stmt_info))
		    mask_producers.safe_push (stmt_info);
		  bool_result = true;

		  if (gimple_code (stmt) == GIMPLE_ASSIGN
		      && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
			 == tcc_comparison
		      && TREE_CODE (TREE_TYPE (gimple_assign_rhs1 (stmt)))
			 != BOOLEAN_TYPE)
		    scalar_type = TREE_TYPE (gimple_assign_rhs1 (stmt));
		  else
		    {
	             if (!analyze_pattern_stmt && gsi_end_p (pattern_def_si))
			{
			  pattern_def_seq = NULL;
			  gsi_next (&si);
			}
		      continue;
		    }
		}

which leaves STMT_VINFO_VECTYPE set to NULL_TREE.  It would have been nice to 
say in the comment why boolean operations don't participate in vectorization 
factor computation; one can only assume that it's because they are somehow 
treated specially, so the proposed fix does the same in vectorizable_reduction 
(and is probably a no-op except for Ada because of the precision test).

Tested on x86_64-suse-linux, OK for mainline and 6 branch?


2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-vect-loop.c (vectorizable_reduction): Also return false if the
	scalar type is a boolean type.


2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt56.ad[sb]: New test.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 988 bytes --]

Index: tree-vect-loop.c
===================================================================
--- tree-vect-loop.c	(revision 236877)
+++ tree-vect-loop.c	(working copy)
@@ -5496,13 +5496,15 @@ vectorizable_reduction (gimple *stmt, gi
 
   scalar_dest = gimple_assign_lhs (stmt);
   scalar_type = TREE_TYPE (scalar_dest);
-  if (!POINTER_TYPE_P (scalar_type) && !INTEGRAL_TYPE_P (scalar_type)
+  if (!POINTER_TYPE_P (scalar_type)
+      && !INTEGRAL_TYPE_P (scalar_type)
       && !SCALAR_FLOAT_TYPE_P (scalar_type))
     return false;
 
-  /* Do not try to vectorize bit-precision reductions.  */
-  if ((TYPE_PRECISION (scalar_type)
-       != GET_MODE_PRECISION (TYPE_MODE (scalar_type))))
+  /* Do not try to vectorize boolean or bit-precision reductions.  */
+  if (TREE_CODE (scalar_type) == BOOLEAN_TYPE
+      || TYPE_PRECISION (scalar_type)
+	 != GET_MODE_PRECISION (TYPE_MODE (scalar_type)))
     return false;
 
   /* All uses but the last are expected to be defined in the loop.

[-- Attachment #3: opt56.adb --]
[-- Type: text/x-adasrc, Size: 297 bytes --]

-- { dg-do compile }
-- { dg-options "-O3" }

package body Opt56 is

   function F (Values : Vector) return Boolean is
      Result : Boolean := True;
   begin
      for I in Values'Range loop
         Result := Result and Values (I) >= 0.0;
     end loop;
     return Result;
   end;

end Opt56;

[-- Attachment #4: opt56.ads --]
[-- Type: text/x-adasrc, Size: 133 bytes --]

package Opt56 is

   type Vector is array (Positive range <>) of Float;

   function F (Values : Vector) return Boolean;

end Opt56;

Re: [patch] Fix segfault in vectorizer

[Richard Biener]

On Tue, May 31, 2016 at 10:22 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> it's a regression present on the mainline and 6 branch: for the attached Ada
> testcase, optab_for_tree_code segfaults because the function is invoked on a
> NULL_TREE vectype_out from the vectorizer (vectorizable_reduction):
>
>   if (STMT_VINFO_VEC_REDUCTION_TYPE (stmt_info) == TREE_CODE_REDUCTION
>       || STMT_VINFO_VEC_REDUCTION_TYPE (stmt_info)
>                 == INTEGER_INDUC_COND_REDUCTION)
>     {
>       if (reduction_code_for_scalar_code (orig_code, &epilog_reduc_code))
>         {
>           reduc_optab = optab_for_tree_code (epilog_reduc_code, vectype_out,
>                                          optab_default);
>
> Naive attempts at working around the NULL_TREE result in bogus vectorization
> and GIMPLE verification failure so it seems clear that vectype_out ought not
> to be NULL_TREE here.
>
> The problems stems from vect_determine_vectorization_factor, namely:
>
>               /* Bool ops don't participate in vectorization factor
>                  computation.  For comparison use compared types to
>                  compute a factor.  */
>               if (TREE_CODE (scalar_type) == BOOLEAN_TYPE
>                   && is_gimple_assign (stmt)
>                   && gimple_assign_rhs_code (stmt) != COND_EXPR)
>                 {
>                   if (STMT_VINFO_RELEVANT_P (stmt_info))
>                     mask_producers.safe_push (stmt_info);
>                   bool_result = true;
>
>                   if (gimple_code (stmt) == GIMPLE_ASSIGN
>                       && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
>                          == tcc_comparison
>                       && TREE_CODE (TREE_TYPE (gimple_assign_rhs1 (stmt)))
>                          != BOOLEAN_TYPE)
>                     scalar_type = TREE_TYPE (gimple_assign_rhs1 (stmt));
>                   else
>                     {
>                      if (!analyze_pattern_stmt && gsi_end_p (pattern_def_si))
>                         {
>                           pattern_def_seq = NULL;
>                           gsi_next (&si);
>                         }
>                       continue;
>                     }
>                 }
>
> which leaves STMT_VINFO_VECTYPE set to NULL_TREE.  It would have been nice to
> say in the comment why boolean operations don't participate in vectorization
> factor computation; one can only assume that it's because they are somehow
> treated specially, so the proposed fix does the same in vectorizable_reduction
> (and is probably a no-op except for Ada because of the precision test).

Note that vect_determine_vectorization_factor is supposed to set the
vector type on all
stmts.  That it doesn't is a bug.  Do you run into the else branch?  I
think that should
only trigger with STMT_VINFO_RELEVANT_P and thus the stmt in mask_producers
which should later get post-processed and have the VECTYPE set.

But maybe Ilya can shed some more light on this (awkward) code.

Richard.

> Tested on x86_64-suse-linux, OK for mainline and 6 branch?
>
>
> 2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-vect-loop.c (vectorizable_reduction): Also return false if the
>         scalar type is a boolean type.
>
>
> 2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt56.ad[sb]: New test.
>
> --
> Eric Botcazou

Re: [patch] Fix segfault in vectorizer

[Eric Botcazou]

> Note that vect_determine_vectorization_factor is supposed to set the
> vector type on all
> stmts.  That it doesn't is a bug.  Do you run into the else branch?

Yes, for

  result_15 = _6 & result_3;

wich is a BIT_AND_EXPR, hence accepted by vectorizable_reduction.

> I think that should only trigger with STMT_VINFO_RELEVANT_P and thus the
> stmt in mask_producers which should later get post-processed and have the
> VECTYPE set.

OK, STMT_VINFO_RELEVANT_P is indeed not set:

(gdb) p *stmt_info
$4 = {type = undef_vec_info_type, live = true, in_pattern_p = false, 
  stmt = 0x7ffff68f5738, vinfo = 0x2e14820, vectype = 0x0, 
  vectorized_stmt = 0x0, data_ref_info = 0x0, dr_base_address = 0x0, 
  dr_init = 0x0, dr_offset = 0x0, dr_step = 0x0, dr_aligned_to = 0x0, 
  loop_phi_evolution_base_unchanged = 0x0, loop_phi_evolution_part = 0x0, 
  related_stmt = 0x0, pattern_def_seq = 0x0, same_align_refs = {m_vec = 0x0}, 
  simd_clone_info = {m_vec = 0x0}, def_type = vect_reduction_def, 
  slp_type = loop_vect, first_element = 0x0, next_element = 0x0, 
  same_dr_stmt = 0x0, size = 0, store_count = 0, gap = 0, min_neg_dist = 0, 
  relevant = vect_unused_in_scope, vectorizable = true, 
  gather_scatter_p = false, strided_p = false, simd_lane_access_p = false, 
  v_reduc_type = TREE_CODE_REDUCTION, num_slp_uses = 0}

-- 
Eric Botcazou

Re: [patch] Fix segfault in vectorizer

[Richard Biener]

On Tue, May 31, 2016 at 11:46 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Note that vect_determine_vectorization_factor is supposed to set the
>> vector type on all
>> stmts.  That it doesn't is a bug.  Do you run into the else branch?
>
> Yes, for
>
>   result_15 = _6 & result_3;
>
> wich is a BIT_AND_EXPR, hence accepted by vectorizable_reduction.
>
>> I think that should only trigger with STMT_VINFO_RELEVANT_P and thus the
>> stmt in mask_producers which should later get post-processed and have the
>> VECTYPE set.
>
> OK, STMT_VINFO_RELEVANT_P is indeed not set:
>
> (gdb) p *stmt_info
> $4 = {type = undef_vec_info_type, live = true, in_pattern_p = false,
>   stmt = 0x7ffff68f5738, vinfo = 0x2e14820, vectype = 0x0,
>   vectorized_stmt = 0x0, data_ref_info = 0x0, dr_base_address = 0x0,
>   dr_init = 0x0, dr_offset = 0x0, dr_step = 0x0, dr_aligned_to = 0x0,
>   loop_phi_evolution_base_unchanged = 0x0, loop_phi_evolution_part = 0x0,
>   related_stmt = 0x0, pattern_def_seq = 0x0, same_align_refs = {m_vec = 0x0},
>   simd_clone_info = {m_vec = 0x0}, def_type = vect_reduction_def,
>   slp_type = loop_vect, first_element = 0x0, next_element = 0x0,
>   same_dr_stmt = 0x0, size = 0, store_count = 0, gap = 0, min_neg_dist = 0,
>   relevant = vect_unused_in_scope, vectorizable = true,
>   gather_scatter_p = false, strided_p = false, simd_lane_access_p = false,
>   v_reduc_type = TREE_CODE_REDUCTION, num_slp_uses = 0}

I recall that some STMT_VINFO_RELEVANT_P checks have a ||
STMT_VINFO_DEF_TYPE () == vect_reduction_def
or VECTORIZABLE_CYCLE_DEF (STMT_VINFO_DEF_TYPE ()).  Maybe that is missing here.

Richard.

> --
> Eric Botcazou

Re: [patch] Fix segfault in vectorizer

[Eric Botcazou]

> I recall that some STMT_VINFO_RELEVANT_P checks have a ||
> STMT_VINFO_DEF_TYPE () == vect_reduction_def
> or VECTORIZABLE_CYCLE_DEF (STMT_VINFO_DEF_TYPE ()).  

It's rather || STMT_VINFO_LIVE_P in most cases and this works here, the 
vectorization is properly blocked:

opt56.adb:9:29: note: not vectorized: different sized masks types in 
statement, vector(16) unsigned char and vector(4) <unnamed type>
opt56.adb:9:29: note: can't determine vectorization factor.
opt56.adb:6:4: note: vectorized 0 loops in function.

-- 
Eric Botcazou

Re: [patch] Fix segfault in vectorizer

[Ilya Enkovich]

2016-05-31 12:25 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Tue, May 31, 2016 at 10:22 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Hi,
>>
>> it's a regression present on the mainline and 6 branch: for the attached Ada
>> testcase, optab_for_tree_code segfaults because the function is invoked on a
>> NULL_TREE vectype_out from the vectorizer (vectorizable_reduction):
>>
>>   if (STMT_VINFO_VEC_REDUCTION_TYPE (stmt_info) == TREE_CODE_REDUCTION
>>       || STMT_VINFO_VEC_REDUCTION_TYPE (stmt_info)
>>                 == INTEGER_INDUC_COND_REDUCTION)
>>     {
>>       if (reduction_code_for_scalar_code (orig_code, &epilog_reduc_code))
>>         {
>>           reduc_optab = optab_for_tree_code (epilog_reduc_code, vectype_out,
>>                                          optab_default);
>>
>> Naive attempts at working around the NULL_TREE result in bogus vectorization
>> and GIMPLE verification failure so it seems clear that vectype_out ought not
>> to be NULL_TREE here.
>>
>> The problems stems from vect_determine_vectorization_factor, namely:
>>
>>               /* Bool ops don't participate in vectorization factor
>>                  computation.  For comparison use compared types to
>>                  compute a factor.  */
>>               if (TREE_CODE (scalar_type) == BOOLEAN_TYPE
>>                   && is_gimple_assign (stmt)
>>                   && gimple_assign_rhs_code (stmt) != COND_EXPR)
>>                 {
>>                   if (STMT_VINFO_RELEVANT_P (stmt_info))
>>                     mask_producers.safe_push (stmt_info);
>>                   bool_result = true;
>>
>>                   if (gimple_code (stmt) == GIMPLE_ASSIGN
>>                       && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
>>                          == tcc_comparison
>>                       && TREE_CODE (TREE_TYPE (gimple_assign_rhs1 (stmt)))
>>                          != BOOLEAN_TYPE)
>>                     scalar_type = TREE_TYPE (gimple_assign_rhs1 (stmt));
>>                   else
>>                     {
>>                      if (!analyze_pattern_stmt && gsi_end_p (pattern_def_si))
>>                         {
>>                           pattern_def_seq = NULL;
>>                           gsi_next (&si);
>>                         }
>>                       continue;
>>                     }
>>                 }
>>
>> which leaves STMT_VINFO_VECTYPE set to NULL_TREE.  It would have been nice to
>> say in the comment why boolean operations don't participate in vectorization
>> factor computation; one can only assume that it's because they are somehow
>> treated specially, so the proposed fix does the same in vectorizable_reduction
>> (and is probably a no-op except for Ada because of the precision test).
>
> Note that vect_determine_vectorization_factor is supposed to set the
> vector type on all
> stmts.  That it doesn't is a bug.  Do you run into the else branch?  I
> think that should
> only trigger with STMT_VINFO_RELEVANT_P and thus the stmt in mask_producers
> which should later get post-processed and have the VECTYPE set.
>
> But maybe Ilya can shed some more light on this (awkward) code.

This code appears when we try to disable boolean patterns.  Boolean patterns
replace booleans with integers of proper size which allow us to simply determine
vectype using get_vectype_for_scalar_type.  With no such replacement we
can't determine vectype just out of a scalar type (there are multiple possible
options and get_vectype_for_scalar_type use would result in a lot of redundant
conversions).  So we delay vectype computation for them and compute it basing on
vectypes computed for their arguments.

Surely any missing vectype for a statement due to this delay is a bug.  I didn't
notice STMT_VINFO_LIVE_P should be checked in addition to STMT_VINFO_RELEVANT_P.

Thanks,
Ilya

>
> Richard.
>
>> Tested on x86_64-suse-linux, OK for mainline and 6 branch?
>>
>>
>> 2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>>
>>         * tree-vect-loop.c (vectorizable_reduction): Also return false if the
>>         scalar type is a boolean type.
>>
>>
>> 2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>>
>>         * gnat.dg/opt56.ad[sb]: New test.
>>
>> --
>> Eric Botcazou

Re: [patch] Fix segfault in vectorizer

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

> This code appears when we try to disable boolean patterns.  Boolean patterns
> replace booleans with integers of proper size which allow us to simply
> determine vectype using get_vectype_for_scalar_type.  With no such
> replacement we can't determine vectype just out of a scalar type (there are
> multiple possible options and get_vectype_for_scalar_type use would result
> in a lot of redundant conversions).  So we delay vectype computation for
> them and compute it basing on vectypes computed for their arguments.

OK, thanks for the explanation.  It would be nice to document that somewhere 
in the code if this isn't already done.

> Surely any missing vectype for a statement due to this delay is a bug.  I
> didn't notice STMT_VINFO_LIVE_P should be checked in addition to
> STMT_VINFO_RELEVANT_P.

That works indeed and generates no regression.  OK for mainline and 6 branch?


2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-vect-loop.c (vect_determine_vectorization_factor): Also take
	into account live statements for mask producers.


2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt56.ad[sb]: New test.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 519 bytes --]

Index: tree-vect-loop.c
===================================================================
--- tree-vect-loop.c	(revision 236877)
+++ tree-vect-loop.c	(working copy)
@@ -441,7 +441,8 @@ vect_determine_vectorization_factor (loo
 		  && is_gimple_assign (stmt)
 		  && gimple_assign_rhs_code (stmt) != COND_EXPR)
 		{
-		  if (STMT_VINFO_RELEVANT_P (stmt_info))
+		  if (STMT_VINFO_RELEVANT_P (stmt_info)
+		      || STMT_VINFO_LIVE_P (stmt_info))
 		    mask_producers.safe_push (stmt_info);
 		  bool_result = true;
 

Re: [patch] Fix segfault in vectorizer

[Richard Biener]

On Tue, May 31, 2016 at 7:46 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This code appears when we try to disable boolean patterns.  Boolean patterns
>> replace booleans with integers of proper size which allow us to simply
>> determine vectype using get_vectype_for_scalar_type.  With no such
>> replacement we can't determine vectype just out of a scalar type (there are
>> multiple possible options and get_vectype_for_scalar_type use would result
>> in a lot of redundant conversions).  So we delay vectype computation for
>> them and compute it basing on vectypes computed for their arguments.
>
> OK, thanks for the explanation.  It would be nice to document that somewhere
> in the code if this isn't already done.
>
>> Surely any missing vectype for a statement due to this delay is a bug.  I
>> didn't notice STMT_VINFO_LIVE_P should be checked in addition to
>> STMT_VINFO_RELEVANT_P.
>
> That works indeed and generates no regression.  OK for mainline and 6 branch?

Yes.

Thanks,
Richard.

>
> 2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-vect-loop.c (vect_determine_vectorization_factor): Also take
>         into account live statements for mask producers.
>
>
> 2016-05-31  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt56.ad[sb]: New test.
>
> --
> Eric Botcazou

Re: [patch] Fix segfault in vectorizer

[Eric Botcazou]

> Surely any missing vectype for a statement due to this delay is a bug.  I
> didn't notice STMT_VINFO_LIVE_P should be checked in addition to
> STMT_VINFO_RELEVANT_P.

In fact this brings back PR tree-opt/68327 on the 6 branch:

(gdb) frame 0
#0  internal_error (gmsgid=gmsgid@entry=0x170cd58 "in %s, at %s:%d")
    at /home/eric/svn/gcc-6-branch/gcc/diagnostic.c:1251
1251    {
(gdb) frame 1
#1  0x000000000101c5c4 in fancy_abort (file=<optimized out>, 
    line=<optimized out>, function=<optimized out>)
    at /home/eric/svn/gcc-6-branch/gcc/diagnostic.c:1327
1327      internal_error ("in %s, at %s:%d", function, trim_filename (file), 
line);
(gdb) frame 2
#2  0x0000000000b75f5e in vect_is_simple_use (operand=0x7ffff6c3f9d8, 
    vinfo=0x1ddb690, def_stmt=0x7fffffffd888, dt=0x7fffffffd86c, 
    vectype=0x7fffffffd878)
    at /home/eric/svn/gcc-6-branch/gcc/tree-vect-stmts.c:8763
8763          gcc_assert (*vectype != NULL_TREE);
(gdb) p debug_gimple_stmt(*def_stmt)
b.0_2 = PHI <b.0_22(4), _17(7)>
$5 = void

because we don't compute STMT_VINFO_VECTYPE for live PHI statement, but only 
for relevant ones, so we need this too:

Index: tree-vect-loop.c
===================================================================
--- tree-vect-loop.c    (revision 236983)
+++ tree-vect-loop.c    (working copy)
@@ -216,7 +216,8 @@ vect_determine_vectorization_factor (loo
 
          gcc_assert (stmt_info);
 
-         if (STMT_VINFO_RELEVANT_P (stmt_info))
+         if (STMT_VINFO_RELEVANT_P (stmt_info)
+             || STMT_VINFO_LIVE_P (stmt_info))
             {
              gcc_assert (!STMT_VINFO_VECTYPE (stmt_info));
               scalar_type = TREE_TYPE (PHI_RESULT (phi));

-- 
Eric Botcazou