[PATCH] Fix PR65701(?), fix typo in vect_enhance_data_refs_alignment

[Richard Biener]

On Fri, 10 Apr 2015, Richard Biener wrote:

> 
> The following patch fixes a typo (I think) which is present since the
> original introduction of the code in vect_enhance_data_refs_alignment.
> 
>   if (do_peeling
>       && all_misalignments_unknown
>       && vect_supportable_dr_alignment (dr0, false))
>     {
>       /* Check if the target requires to prefer stores over loads, i.e., 
> if
>          misaligned stores are more expensive than misaligned loads 
> (taking
>          drs with same alignment into account).  */
>       if (first_store && DR_IS_READ (dr0))
>         {
> ...
>         }
> 
>       /* In case there are only loads with different unknown 
> misalignments, use
>          peeling only if it may help to align other accesses in the loop.  
> */
>       if (!first_store
>           && !STMT_VINFO_SAME_ALIGN_REFS (
>                   vinfo_for_stmt (DR_STMT (dr0))).length ()
>           && vect_supportable_dr_alignment (dr0, false)
>               != dr_unaligned_supported)
>         do_peeling = false;
>     }
> 
> the last vect_supportable_dr_alignment check doesn't make much sense.
> It's not mentioned in the comment and I see no reason for treating
> dr_unaligned_supported any different here compared to
> dr_explicit_realign_[optimized].

Ok, as always a second after you hit <send> you think of the reason
of this test.  I suppose the idea was that aligning a single load
might improve load bandwith (same reason we eventually prefer
aligning stores).  dr_explicit_realign_[optimized] perform
aligned loads thus it makes sense to special-case them.
I suppose the cost model in that case should be more precise
(and note that for bdver2 aligned and unaligned loads have the
same cost).

So sth like

      /* In case there are only loads with different unknown 
misalignments, use
         peeling only if it may help to align other accesses in the loop 
or
         if it may help improving load bandwith when we'd end up using
         unaligned loads.  */
      tree dr0_vt = STMT_VINFO_VECTYPE (vinfo_for_stmt (DR_STMT (dr0)));
      if (!first_store
          && !STMT_VINFO_SAME_ALIGN_REFS (
                  vinfo_for_stmt (DR_STMT (dr0))).length ()
          && (vect_supportable_dr_alignment (dr0, false)
              != dr_unaligned_supported
              || (builtin_vectorization_cost (vector_load, dr0_vt, 0)
                  == builtin_vectorization_cost (unaligned_load, dr0_vt, 
-1))))
        do_peeling = false;

Looks like all Intel CPUs still have larger unaligned load cost
compared to aligned load cost (same is true for bdver2 in principle -
the cost isn't about the instruction encoding but the actual
data alignment).  So it might be artificially fixing 187.facerec with
bdver2 (we don't have a good idea whether we'll going to be
store or load bandwith limited in the vectorizer).

Richard.

> What would make sense here
> would be a test against dr_unaligned_unsupported (thus, typo), but
> that is already tested in the condition guarding all the code.
> 
> Thus I am testing (and benchmarking) the following patch which
> avoids peeling loads for alignment on targets with support for unaligned
> loads if that peeling would only align a single data reference.
> 
> Bootstrap and regtest in progress on x86_64-unknown-linux-gnu.
> 
> Queued for GCC 6 and eventually GCC 5.2 (if it fixes the regression).
> 
> Richard.
> 
> 2015-04-10  Richard Biener  <rguenther@suse.de>
> 
> 	PR tree-optimization/65701
> 	* tree-vect-data-refs.c (vect_enhance_data_refs_alignment):
> 	Fix typo and then remove redundant test.
> 
> Index: gcc/tree-vect-data-refs.c
> ===================================================================
> --- gcc/tree-vect-data-refs.c	(revision 221971)
> +++ gcc/tree-vect-data-refs.c	(working copy)
> @@ -1618,9 +1608,7 @@ vect_enhance_data_refs_alignment (loop_v
>           peeling only if it may help to align other accesses in the loop.  */
>        if (!first_store
>  	  && !STMT_VINFO_SAME_ALIGN_REFS (
> -		  vinfo_for_stmt (DR_STMT (dr0))).length ()
> -          && vect_supportable_dr_alignment (dr0, false)
> -              != dr_unaligned_supported)
> +		  vinfo_for_stmt (DR_STMT (dr0))).length ())
>          do_peeling = false;
>      }
>  
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)