Submitter | Ira Rosen |
---|---|

Date | Oct. 11, 2010, 1:03 p.m. |

Message ID | <OFA2C5EF1B.57DA87D7-ONC22577B9.00450843-C22577B9.0047BE43@il.ibm.com> |

Download | mbox | patch |

Permalink | /patch/67417/ |

State | New |

Headers | show |

## Comments

On Mon, Oct 11, 2010 at 3:03 PM, Ira Rosen <IRAR@il.ibm.com> wrote: > Richard Guenther <richard.guenther@gmail.com> wrote on 11/10/2010 02:32:08 > PM: > >> >> On Mon, Oct 11, 2010 at 2:19 PM, Ira Rosen <IRAR@il.ibm.com> wrote: >> > >> > Hi, >> > >> > This patch fixes a bug in creation of a vector of constants in SLP. The >> > problem is in the type of created vector. It should be set to the > vector >> > type of the statement unless it's a pointer. >> > >> > Bootstrapped and tested on x86_64-suse-linux and checked that the > failure >> > is fixed on powerpc64-suse-linux. >> > >> > Committed to mainline, ok for 4.5? >> >> I don't think this makes sense. If at all the selection of which type >> to use should be based on the operation code and the operand >> position, but not on CONSTANT_CLASS_P or pointer-type-ness. >> >> So - what operation code / operand position is currently mishandled? > > This function creates vectors of constants or loop invariants. If it's a > variable (loop invariant), we know exactly the type, hence the check for > CONSTANT_CLASS_P. > > I guess instead of checking the type we can check if it's POINTER_PLUS_EXPR > to catch the cases where we need the operand's type instead of the stmt's > vectype. > > Does this make sense? That makes more sense, with ... > Index: tree-vect-slp.c > =================================================================== > --- tree-vect-slp.c (revision 165302) > +++ tree-vect-slp.c (working copy) > @@ -1836,10 +1836,10 @@ vect_get_constant_vectors (slp_tree slp_ > VEC (tree, heap) *voprnds = VEC_alloc (tree, heap, number_of_vectors); > bool constant_p, is_store; > tree neutral_op = NULL; > + enum tree_code code = gimple_assign_rhs_code (stmt); > > if (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def) > { > - enum tree_code code = gimple_assign_rhs_code (stmt); > if (reduc_index == -1) > { > VEC_free (tree, heap, *vec_oprnds); > @@ -1895,18 +1895,14 @@ vect_get_constant_vectors (slp_tree slp_ > } > > if (CONSTANT_CLASS_P (op)) > - { > - constant_p = true; > - if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))) > - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); > - else > - vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); > - } > + constant_p = true; > else > - { > - constant_p = false; > - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); > - } > + constant_p = false; > + > + if (code == POINTER_PLUS_EXPR) checking && op_num == 2 (with a possible fixup for the reduction case where that doesn't seem to be prevailing). Thanks, Richard. > + vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); > + else > + vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); > > gcc_assert (vector_type); > nunits = TYPE_VECTOR_SUBPARTS (vector_type); > > Thanks, > Ira > > >

Richard Guenther <richard.guenther@gmail.com> wrote on 11/10/2010 04:03:07 PM: > > On Mon, Oct 11, 2010 at 3:03 PM, Ira Rosen <IRAR@il.ibm.com> wrote: > > Richard Guenther <richard.guenther@gmail.com> wrote on 11/10/2010 02:32:08 > > PM: > > > >> > >> On Mon, Oct 11, 2010 at 2:19 PM, Ira Rosen <IRAR@il.ibm.com> wrote: > >> > > >> > Hi, > >> > > >> > This patch fixes a bug in creation of a vector of constants in SLP. The > >> > problem is in the type of created vector. It should be set to the > > vector > >> > type of the statement unless it's a pointer. > >> > > >> > Bootstrapped and tested on x86_64-suse-linux and checked that the > > failure > >> > is fixed on powerpc64-suse-linux. > >> > > >> > Committed to mainline, ok for 4.5? > >> > >> I don't think this makes sense. If at all the selection of which type > >> to use should be based on the operation code and the operand > >> position, but not on CONSTANT_CLASS_P or pointer-type-ness. > >> > >> So - what operation code / operand position is currently mishandled? > > > > This function creates vectors of constants or loop invariants. If it's a > > variable (loop invariant), we know exactly the type, hence the check for > > CONSTANT_CLASS_P. > > > > I guess instead of checking the type we can check if it's POINTER_PLUS_EXPR > > to catch the cases where we need the operand's type instead of the stmt's > > vectype. > > > > Does this make sense? > > That makes more sense, with ... > > > Index: tree-vect-slp.c > > =================================================================== > > --- tree-vect-slp.c (revision 165302) > > +++ tree-vect-slp.c (working copy) > > @@ -1836,10 +1836,10 @@ vect_get_constant_vectors (slp_tree slp_ > > VEC (tree, heap) *voprnds = VEC_alloc (tree, heap, number_of_vectors); > > bool constant_p, is_store; > > tree neutral_op = NULL; > > + enum tree_code code = gimple_assign_rhs_code (stmt); > > > > if (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def) > > { > > - enum tree_code code = gimple_assign_rhs_code (stmt); > > if (reduc_index == -1) > > { > > VEC_free (tree, heap, *vec_oprnds); > > @@ -1895,18 +1895,14 @@ vect_get_constant_vectors (slp_tree slp_ > > } > > > > if (CONSTANT_CLASS_P (op)) > > - { > > - constant_p = true; > > - if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))) > > - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); > > - else > > - vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); > > - } > > + constant_p = true; > > else > > - { > > - constant_p = false; > > - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); > > - } > > + constant_p = false; > > + > > + if (code == POINTER_PLUS_EXPR) > > checking > > && op_num == 2 But if it's the first operand then its type is the type of the stmt, i.e., we get the same type anyway, right? Thanks, Ira > > (with a possible fixup for the reduction case where that doesn't seem to > be prevailing). > > Thanks, > Richard. > > > + vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); > > + else > > + vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); > > > > gcc_assert (vector_type); > > nunits = TYPE_VECTOR_SUBPARTS (vector_type); > > > > Thanks, > > Ira > > > > > >

On Mon, Oct 11, 2010 at 10:48 PM, Ira Rosen <IRAR@il.ibm.com> wrote: > > > Richard Guenther <richard.guenther@gmail.com> wrote on 11/10/2010 04:03:07 > PM: > >> >> On Mon, Oct 11, 2010 at 3:03 PM, Ira Rosen <IRAR@il.ibm.com> wrote: >> > Richard Guenther <richard.guenther@gmail.com> wrote on 11/10/2010 > 02:32:08 >> > PM: >> > >> >> >> >> On Mon, Oct 11, 2010 at 2:19 PM, Ira Rosen <IRAR@il.ibm.com> wrote: >> >> > >> >> > Hi, >> >> > >> >> > This patch fixes a bug in creation of a vector of constants in SLP. > The >> >> > problem is in the type of created vector. It should be set to the >> > vector >> >> > type of the statement unless it's a pointer. >> >> > >> >> > Bootstrapped and tested on x86_64-suse-linux and checked that the >> > failure >> >> > is fixed on powerpc64-suse-linux. >> >> > >> >> > Committed to mainline, ok for 4.5? >> >> >> >> I don't think this makes sense. If at all the selection of which type >> >> to use should be based on the operation code and the operand >> >> position, but not on CONSTANT_CLASS_P or pointer-type-ness. >> >> >> >> So - what operation code / operand position is currently mishandled? >> > >> > This function creates vectors of constants or loop invariants. If it's > a >> > variable (loop invariant), we know exactly the type, hence the check > for >> > CONSTANT_CLASS_P. >> > >> > I guess instead of checking the type we can check if it's > POINTER_PLUS_EXPR >> > to catch the cases where we need the operand's type instead of the > stmt's >> > vectype. >> > >> > Does this make sense? >> >> That makes more sense, with ... >> >> > Index: tree-vect-slp.c >> > =================================================================== >> > --- tree-vect-slp.c (revision 165302) >> > +++ tree-vect-slp.c (working copy) >> > @@ -1836,10 +1836,10 @@ vect_get_constant_vectors (slp_tree slp_ >> > VEC (tree, heap) *voprnds = VEC_alloc (tree, heap, > number_of_vectors); >> > bool constant_p, is_store; >> > tree neutral_op = NULL; >> > + enum tree_code code = gimple_assign_rhs_code (stmt); >> > >> > if (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def) >> > { >> > - enum tree_code code = gimple_assign_rhs_code (stmt); >> > if (reduc_index == -1) >> > { >> > VEC_free (tree, heap, *vec_oprnds); >> > @@ -1895,18 +1895,14 @@ vect_get_constant_vectors (slp_tree slp_ >> > } >> > >> > if (CONSTANT_CLASS_P (op)) >> > - { >> > - constant_p = true; >> > - if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))) >> > - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); >> > - else >> > - vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); >> > - } >> > + constant_p = true; >> > else >> > - { >> > - constant_p = false; >> > - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); >> > - } >> > + constant_p = false; >> > + >> > + if (code == POINTER_PLUS_EXPR) >> >> checking >> >> && op_num == 2 > > But if it's the first operand then its type is the type of the stmt, i.e., > we get the same type anyway, right? Yes. So your patch looks ok, possibly with a comment that looking at either operand is ok. Thanks, Richard. > Thanks, > Ira > >> >> (with a possible fixup for the reduction case where that doesn't seem to >> be prevailing). >> >> Thanks, >> Richard. >> >> > + vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); >> > + else >> > + vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); >> > >> > gcc_assert (vector_type); >> > nunits = TYPE_VECTOR_SUBPARTS (vector_type); >> > >> > Thanks, >> > Ira >> > >> > >> > > >

## Patch

Index: tree-vect-slp.c =================================================================== --- tree-vect-slp.c (revision 165302) +++ tree-vect-slp.c (working copy) @@ -1836,10 +1836,10 @@ vect_get_constant_vectors (slp_tree slp_ VEC (tree, heap) *voprnds = VEC_alloc (tree, heap, number_of_vectors); bool constant_p, is_store; tree neutral_op = NULL; + enum tree_code code = gimple_assign_rhs_code (stmt); if (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def) { - enum tree_code code = gimple_assign_rhs_code (stmt); if (reduc_index == -1) { VEC_free (tree, heap, *vec_oprnds); @@ -1895,18 +1895,14 @@ vect_get_constant_vectors (slp_tree slp_ } if (CONSTANT_CLASS_P (op)) - { - constant_p = true; - if (POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))) - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); - else - vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); - } + constant_p = true; else - { - constant_p = false; - vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); - } + constant_p = false; + + if (code == POINTER_PLUS_EXPR) + vector_type = get_vectype_for_scalar_type (TREE_TYPE (op)); + else + vector_type = STMT_VINFO_VECTYPE (stmt_vinfo); gcc_assert (vector_type); nunits = TYPE_VECTOR_SUBPARTS (vector_type);