Re: [PATCH] Fix PR68306

Re: [PATCH] Fix PR68306

[Uros Bizjak]

Hello!

> 2015-11-13  Richard Biener  <rguenther@suse.de>
>
> PR tree-optimization/68306
> * tree-vect-data-refs.c (verify_data_ref_alignment): Move
> loop related checks ...
> (vect_verify_datarefs_alignment): ... here.
> (vect_slp_analyze_and_verify_node_alignment): Compute and
> verify alignment of the single DR that it matters.
> * tree-vect-stmts.c (vectorizable_store): Add an assert.
> (vectorizable_load): Add a comment.
> * tree-vect-slp.c (vect_analyze_slp_cost_1): Fix DR used
> for determining load cost.
>
> * gcc.dg/pr68306.c: Adjust.
> * gcc.dg/pr68306-2.c: New testcase.
> * gcc.dg/pr68306-3.c: Likewise.

+ /* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */

You should use  { target i?86-*-* x86_64-*-* } here and in a couple of
other places.

Uros.

Re: [PATCH] Fix PR68306

[Uros Bizjak]

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

On Fri, Nov 13, 2015 at 2:15 PM, Uros Bizjak <ubizjak@gmail.com> wrote:

>> 2015-11-13  Richard Biener  <rguenther@suse.de>
>>
>> PR tree-optimization/68306
>> * tree-vect-data-refs.c (verify_data_ref_alignment): Move
>> loop related checks ...
>> (vect_verify_datarefs_alignment): ... here.
>> (vect_slp_analyze_and_verify_node_alignment): Compute and
>> verify alignment of the single DR that it matters.
>> * tree-vect-stmts.c (vectorizable_store): Add an assert.
>> (vectorizable_load): Add a comment.
>> * tree-vect-slp.c (vect_analyze_slp_cost_1): Fix DR used
>> for determining load cost.
>>
>> * gcc.dg/pr68306.c: Adjust.
>> * gcc.dg/pr68306-2.c: New testcase.
>> * gcc.dg/pr68306-3.c: Likewise.
>
> + /* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
>
> You should use  { target i?86-*-* x86_64-*-* } here and in a couple of
> other places.

Added by attached patch.

2015-11-13  Uros Bizjak  <ubizjak@gmail.com>

    * gcc.dg/pr68306.c (dg-additional-options): Add i?86-*-* target.
    * gcc.dg/pr68306-2.c (dg-additional-options): Ditto.
    * gcc.dg/pr68306-3.c (dg-additional-options): Ditto.

Tested on x86_64-linux-gnu {,-m32}  and committed to mainline SVN.

Uros.

[-- Attachment #2: t.diff.txt --]
[-- Type: text/plain, Size: 1878 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 230338)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2015-11-13  Uros Bizjak  <ubizjak@gmail.com>
+
+	* gcc.dg/pr68306.c (dg-additional-options): Add i?86-*-* target.
+	* gcc.dg/pr68306-2.c (dg-additional-options): Ditto.
+	* gcc.dg/pr68306-3.c (dg-additional-options): Ditto.
+
 2015-11-13  David Malcolm  <dmalcolm@redhat.com>
 
 	* gcc.dg/diagnostic-token-ranges.c: New file.
Index: gcc.dg/pr68306-2.c
===================================================================
--- gcc.dg/pr68306-2.c	(revision 230338)
+++ gcc.dg/pr68306-2.c	(working copy)
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O3" } */
-/* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
+/* { dg-additional-options "-mno-sse -mno-mmx" { target i?86-*-* x86_64-*-* } } */
 
 struct {
     int tz_minuteswest;
Index: gcc.dg/pr68306-3.c
===================================================================
--- gcc.dg/pr68306-3.c	(revision 230338)
+++ gcc.dg/pr68306-3.c	(working copy)
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O3" } */
-/* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
+/* { dg-additional-options "-mno-sse -mno-mmx" { target i?86-*-* x86_64-*-* } } */
 /* { dg-additional-options "-mno-altivec -mno-vsx" { target powerpc*-*-* } } */
 
 extern void fn2();
Index: gcc.dg/pr68306.c
===================================================================
--- gcc.dg/pr68306.c	(revision 230338)
+++ gcc.dg/pr68306.c	(working copy)
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-options "-O3" } */
-/* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
+/* { dg-additional-options "-mno-sse -mno-mmx" { target i?86-*-* x86_64-*-* } } */
 
 enum powerpc_pmc_type { PPC_PMC_IBM };
 struct {

[PATCH] Fix PR68306

[Richard Biener]

This fixes more of PR68306.  Digging into the details reveals
that we only need to check a single (but the correct one) DR
and that cost modeling also gets this wrong.

Thus fixed.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied.

Richard.

2015-11-13  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/68306
	* tree-vect-data-refs.c (verify_data_ref_alignment): Move
	loop related checks ...
	(vect_verify_datarefs_alignment): ... here.
	(vect_slp_analyze_and_verify_node_alignment): Compute and
	verify alignment of the single DR that it matters.
	* tree-vect-stmts.c (vectorizable_store): Add an assert.
	(vectorizable_load): Add a comment.
	* tree-vect-slp.c (vect_analyze_slp_cost_1): Fix DR used
	for determining load cost.

	* gcc.dg/pr68306.c: Adjust.
	* gcc.dg/pr68306-2.c: New testcase.
	* gcc.dg/pr68306-3.c: Likewise.

Index: gcc/tree-vect-data-refs.c
===================================================================
*** gcc/tree-vect-data-refs.c	(revision 230293)
--- gcc/tree-vect-data-refs.c	(working copy)
*************** verify_data_ref_alignment (data_referenc
*** 920,936 ****
    gimple *stmt = DR_STMT (dr);
    stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
  
-   /* For interleaving, only the alignment of the first access matters.   */
-   if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
-       && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
-     return true;
- 
-   /* Strided accesses perform only component accesses, alignment is
-      irrelevant for them.  */
-   if (STMT_VINFO_STRIDED_P (stmt_info)
-       && !STMT_VINFO_GROUPED_ACCESS (stmt_info))
-     return true;
- 
    supportable_dr_alignment = vect_supportable_dr_alignment (dr, false);
    if (!supportable_dr_alignment)
      {
--- 920,925 ----
*************** vect_verify_datarefs_alignment (loop_vec
*** 977,982 ****
--- 966,983 ----
  
        if (!STMT_VINFO_RELEVANT_P (stmt_info))
  	continue;
+ 
+       /* For interleaving, only the alignment of the first access matters.   */
+       if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
+ 	  && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
+ 	return true;
+ 
+       /* Strided accesses perform only component accesses, alignment is
+ 	 irrelevant for them.  */
+       if (STMT_VINFO_STRIDED_P (stmt_info)
+ 	  && !STMT_VINFO_GROUPED_ACCESS (stmt_info))
+ 	return true;
+ 
        if (! verify_data_ref_alignment (dr))
  	return false;
      }
*************** vect_analyze_data_refs_alignment (loop_v
*** 2100,2127 ****
  static bool
  vect_slp_analyze_and_verify_node_alignment (slp_tree node)
  {
!   unsigned i;
!   gimple *stmt;
!   FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_STMTS (node), i, stmt)
      {
!       stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
! 
!       /* Strided accesses perform only component accesses, misalignment
! 	 information is irrelevant for them.  */
!       if (STMT_VINFO_STRIDED_P (stmt_info)
! 	  && !STMT_VINFO_GROUPED_ACCESS (stmt_info))
! 	continue;
! 
!       data_reference_p dr = STMT_VINFO_DATA_REF (stmt_info);
!       if (! vect_compute_data_ref_alignment (dr)
! 	  || ! verify_data_ref_alignment (dr))
! 	{
! 	  if (dump_enabled_p ())
! 	    dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
! 			     "not vectorized: bad data alignment in basic "
! 			     "block.\n");
! 	  return false;
! 	}
      }
  
    return true;
--- 2101,2122 ----
  static bool
  vect_slp_analyze_and_verify_node_alignment (slp_tree node)
  {
!   /* We vectorize from the first scalar stmt in the node unless
!      the node is permuted in which case we start from the first
!      element in the group.  */
!   gimple *first_stmt = SLP_TREE_SCALAR_STMTS (node)[0];
!   if (SLP_TREE_LOAD_PERMUTATION (node).exists ())
!     first_stmt = GROUP_FIRST_ELEMENT (vinfo_for_stmt (first_stmt));
! 
!   data_reference_p dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
!   if (! vect_compute_data_ref_alignment (dr)
!       || ! verify_data_ref_alignment (dr))
      {
!       if (dump_enabled_p ())
! 	dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
! 			 "not vectorized: bad data alignment in basic "
! 			 "block.\n");
!       return false;
      }
  
    return true;
Index: gcc/tree-vect-stmts.c
===================================================================
*** gcc/tree-vect-stmts.c	(revision 230293)
--- gcc/tree-vect-stmts.c	(working copy)
*************** vectorizable_store (gimple *stmt, gimple
*** 5464,5469 ****
--- 5464,5470 ----
               group.  */
            vec_num = SLP_TREE_NUMBER_OF_VEC_STMTS (slp_node);
            first_stmt = SLP_TREE_SCALAR_STMTS (slp_node)[0]; 
+ 	  gcc_assert (GROUP_FIRST_ELEMENT (vinfo_for_stmt (first_stmt)) == first_stmt);
            first_dr = STMT_VINFO_DATA_REF (vinfo_for_stmt (first_stmt));
  	  op = gimple_assign_rhs1 (first_stmt);
          } 
*************** vectorizable_load (gimple *stmt, gimple_
*** 6658,6666 ****
    if (grouped_load)
      {
        first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
!       if (slp
!           && !SLP_TREE_LOAD_PERMUTATION (slp_node).exists ()
! 	  && first_stmt != SLP_TREE_SCALAR_STMTS (slp_node)[0])
          first_stmt = SLP_TREE_SCALAR_STMTS (slp_node)[0];
  
        /* Check if the chain of loads is already vectorized.  */
--- 6659,6667 ----
    if (grouped_load)
      {
        first_stmt = GROUP_FIRST_ELEMENT (stmt_info);
!       /* For BB vectorization we directly vectorize a subchain
!          without permutation.  */
!       if (slp && ! SLP_TREE_LOAD_PERMUTATION (slp_node).exists ())
          first_stmt = SLP_TREE_SCALAR_STMTS (slp_node)[0];
  
        /* Check if the chain of loads is already vectorized.  */
Index: gcc/tree-vect-slp.c
===================================================================
*** gcc/tree-vect-slp.c	(revision 230293)
--- gcc/tree-vect-slp.c	(working copy)
*************** vect_analyze_slp_cost_1 (slp_instance in
*** 1429,1434 ****
--- 1429,1441 ----
  	{
  	  int i;
  	  gcc_checking_assert (DR_IS_READ (STMT_VINFO_DATA_REF (stmt_info)));
+ 	  /* If the load is permuted then the alignment is determined by
+ 	     the first group element not by the first scalar stmt DR.  */
+ 	  if (SLP_TREE_LOAD_PERMUTATION (node).exists ())
+ 	    {
+ 	      stmt = GROUP_FIRST_ELEMENT (stmt_info);
+ 	      stmt_info = vinfo_for_stmt (stmt);
+ 	    }
  	  vect_model_load_cost (stmt_info, ncopies_for_cost, false,
  				node, prologue_cost_vec, body_cost_vec);
  	  /* If the load is permuted record the cost for the permutation.
Index: gcc/testsuite/gcc.dg/pr68306.c
===================================================================
*** gcc/testsuite/gcc.dg/pr68306.c	(revision 230293)
--- gcc/testsuite/gcc.dg/pr68306.c	(working copy)
***************
*** 1,5 ****
--- 1,6 ----
  /* { dg-do compile } */
  /* { dg-options "-O3" } */
+ /* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
  
  enum powerpc_pmc_type { PPC_PMC_IBM };
  struct {
Index: gcc/testsuite/gcc.dg/pr68306-2.c
===================================================================
*** gcc/testsuite/gcc.dg/pr68306-2.c	(revision 0)
--- gcc/testsuite/gcc.dg/pr68306-2.c	(revision 0)
***************
*** 0 ****
--- 1,12 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O3" } */
+ /* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
+ 
+ struct {
+     int tz_minuteswest;
+     int tz_dsttime;
+ } a, b;
+ void fn1() {
+     b.tz_minuteswest = a.tz_minuteswest;
+     b.tz_dsttime = a.tz_dsttime;
+ }
Index: gcc/testsuite/gcc.dg/pr68306-3.c
===================================================================
*** gcc/testsuite/gcc.dg/pr68306-3.c	(revision 0)
--- gcc/testsuite/gcc.dg/pr68306-3.c	(revision 0)
***************
*** 0 ****
--- 1,21 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O3" } */
+ /* { dg-additional-options "-mno-sse -mno-mmx" { target x86_64-*-* } } */
+ /* { dg-additional-options "-mno-altivec -mno-vsx" { target powerpc*-*-* } } */
+ 
+ extern void fn2();
+ struct {
+     unsigned qp_num;
+     unsigned starting_psn;
+     void *private_data;
+ } a;
+ struct {
+     unsigned id;
+     unsigned qpn;
+     unsigned psn;
+ } b;
+ void fn1() {
+     a.qp_num = b.qpn;
+     a.starting_psn = b.psn;
+     fn2(b.id);
+ }

[PATCH] Fix PR68306

[Richard Biener]

The following fixes PR68306, an ordering issue with my last BB
vectorization patch.  Fixed by removing that ordering requirement.

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

Richard.

2015-11-12  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/68306
	* tree-vect-data-refs.c (verify_data_ref_alignment): Remove
	relevant and vectorizable checks here.
	(vect_verify_datarefs_alignment): Add relevant check here.

	* gcc.dg/pr68306.c: New testcase.

Index: gcc/tree-vect-data-refs.c
===================================================================
*** gcc/tree-vect-data-refs.c	(revision 230216)
--- gcc/tree-vect-data-refs.c	(working copy)
*************** verify_data_ref_alignment (data_referenc
*** 909,922 ****
    gimple *stmt = DR_STMT (dr);
    stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
  
!   if (!STMT_VINFO_RELEVANT_P (stmt_info))
!     return true;
! 
!   /* For interleaving, only the alignment of the first access matters. 
!      Skip statements marked as not vectorizable.  */
!   if ((STMT_VINFO_GROUPED_ACCESS (stmt_info)
!        && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
!       || !STMT_VINFO_VECTORIZABLE (stmt_info))
      return true;
  
    /* Strided accesses perform only component accesses, alignment is
--- 889,897 ----
    gimple *stmt = DR_STMT (dr);
    stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
  
!   /* For interleaving, only the alignment of the first access matters.   */
!   if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
!       && GROUP_FIRST_ELEMENT (stmt_info) != stmt)
      return true;
  
    /* Strided accesses perform only component accesses, alignment is
*************** vect_verify_datarefs_alignment (loop_vec
*** 965,972 ****
    unsigned int i;
  
    FOR_EACH_VEC_ELT (datarefs, i, dr)
!     if (! verify_data_ref_alignment (dr))
!       return false;
  
    return true;
  }
--- 940,954 ----
    unsigned int i;
  
    FOR_EACH_VEC_ELT (datarefs, i, dr)
!     {
!       gimple *stmt = DR_STMT (dr);
!       stmt_vec_info stmt_info = vinfo_for_stmt (stmt);
! 
!       if (!STMT_VINFO_RELEVANT_P (stmt_info))
! 	continue;
!       if (! verify_data_ref_alignment (dr))
! 	return false;
!     }
  
    return true;
  }
Index: gcc/testsuite/gcc.dg/pr68306.c
===================================================================
*** gcc/testsuite/gcc.dg/pr68306.c	(revision 0)
--- gcc/testsuite/gcc.dg/pr68306.c	(working copy)
***************
*** 0 ****
--- 1,10 ----
+ /* { dg-do compile } */
+ /* { dg-options "-O3" } */
+ 
+ enum powerpc_pmc_type { PPC_PMC_IBM };
+ struct {
+     unsigned num_pmcs;
+     enum powerpc_pmc_type pmc_type;
+ } a;
+ enum powerpc_pmc_type b;
+ void fn1() { a.num_pmcs = a.pmc_type = b; }