| Message ID | alpine.DEB.2.10.1306271344370.26429@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On 06/27/2013 07:59 AM, Marc Glisse wrote: > I assume I can't call directly c_build_vec_perm_expr on the original > arguments without build_non_dependent_expr? It looks like c_build_vec_perm_expr is safe to take the original arguments, since it doesn't look deep into the expression. So either way is fine. > By the way, should I rename cp_build_vec_perm_expr as build_x_vec_perm_expr, since most of its code is copied from build_x_binary_op and not cp_build_binary_op? Makes sense. OK with that change. Jason
On Thu, 27 Jun 2013, Jason Merrill wrote: > On 06/27/2013 07:59 AM, Marc Glisse wrote: >> I assume I can't call directly c_build_vec_perm_expr on the original >> arguments without build_non_dependent_expr? > > It looks like c_build_vec_perm_expr is safe to take the original arguments, > since it doesn't look deep into the expression. So either way is fine. Cool, I'll go with the short version then (I tested it before posting): +tree +build_x_vec_perm_expr (location_t loc, + tree arg0, tree arg1, tree arg2, + tsubst_flags_t complain) +{ + if (processing_template_decl + && (type_dependent_expression_p (arg0) + || type_dependent_expression_p (arg1) + || type_dependent_expression_p (arg2))) + return build_min_nt_loc (loc, VEC_PERM_EXPR, arg0, arg1, arg2); + return c_build_vec_perm_expr (loc, arg0, arg1, arg2, complain & tf_error); +} >> By the way, should I rename cp_build_vec_perm_expr as >> build_x_vec_perm_expr, since most of its code is copied from >> build_x_binary_op and not cp_build_binary_op? > > Makes sense. OK with that change. Thanks.
On Thu, Jun 27, 2013 at 05:24:45PM +0200, Marc Glisse wrote: > On Thu, 27 Jun 2013, Jason Merrill wrote: > > >On 06/27/2013 07:59 AM, Marc Glisse wrote: > >>I assume I can't call directly c_build_vec_perm_expr on the original > >>arguments without build_non_dependent_expr? > > > >It looks like c_build_vec_perm_expr is safe to take the original > >arguments, since it doesn't look deep into the expression. So > >either way is fine. > > Cool, I'll go with the short version then (I tested it before posting): > > +tree > +build_x_vec_perm_expr (location_t loc, > + tree arg0, tree arg1, tree arg2, > + tsubst_flags_t complain) > +{ > + if (processing_template_decl > + && (type_dependent_expression_p (arg0) > + || type_dependent_expression_p (arg1) > + || type_dependent_expression_p (arg2))) > + return build_min_nt_loc (loc, VEC_PERM_EXPR, arg0, arg1, arg2); > + return c_build_vec_perm_expr (loc, arg0, arg1, arg2, complain & tf_error); > +} But then you won't diagnose errors in never instantiated templates that could be diagnosed (i.e. where the arguments aren't type dependent). I think the standard C++ FE way is doing something like: + tree expr; + tree orig_arg0 = arg0; + tree orig_arg1 = arg1; + tree orig_arg2 = arg2; + if (processing_template_decl) + { + if (type_dependent_expression_p (arg0) + || type_dependent_expression_p (arg1) + || type_dependent_expression_p (arg2)) + return build_min_nt_loc (loc, VEC_PERM_EXPR, arg0, arg1, arg2); + arg0 = build_non_dependent_expr (arg0); + arg1 = build_non_dependent_expr (arg1); + arg2 = build_non_dependent_expr (arg2); + } + expr = c_build_vec_perm_expr (loc, arg0, arg1, arg2, complain & tf_error); + if (processing_template_decl && expr != error_mark_node) + return build_min_nt_loc (loc, VEC_PERM_EXPR, orig_arg0, orig_arg1, + orig_arg2); + return expr; Jakub
On Thu, 27 Jun 2013, Jakub Jelinek wrote: > On Thu, Jun 27, 2013 at 05:24:45PM +0200, Marc Glisse wrote: >> On Thu, 27 Jun 2013, Jason Merrill wrote: >> >>> On 06/27/2013 07:59 AM, Marc Glisse wrote: >>>> I assume I can't call directly c_build_vec_perm_expr on the original >>>> arguments without build_non_dependent_expr? >>> >>> It looks like c_build_vec_perm_expr is safe to take the original >>> arguments, since it doesn't look deep into the expression. So >>> either way is fine. >> >> Cool, I'll go with the short version then (I tested it before posting): >> >> +tree >> +build_x_vec_perm_expr (location_t loc, >> + tree arg0, tree arg1, tree arg2, >> + tsubst_flags_t complain) >> +{ >> + if (processing_template_decl >> + && (type_dependent_expression_p (arg0) >> + || type_dependent_expression_p (arg1) >> + || type_dependent_expression_p (arg2))) >> + return build_min_nt_loc (loc, VEC_PERM_EXPR, arg0, arg1, arg2); >> + return c_build_vec_perm_expr (loc, arg0, arg1, arg2, complain & tf_error); >> +} > > But then you won't diagnose errors in never instantiated templates that > could be diagnosed (i.e. where the arguments aren't type dependent). I don't really see why, as I am still calling c_build_vec_perm_expr in the same cases, just possibly not exactly with the same arguments (they don't go through build_non_dependent_expr, but Jason seemed to imply that it did not matter since we don't look too deep through the arguments). > I think the standard C++ FE way is doing something like: [snip previous version that Jason accepted] If you prefer the long version, I don't mind going back to it.
On Thu, Jun 27, 2013 at 05:54:10PM +0200, Marc Glisse wrote: > I don't really see why, as I am still calling c_build_vec_perm_expr > in the same cases, just possibly not exactly with the same arguments > (they don't go through build_non_dependent_expr, but Jason seemed to > imply that it did not matter since we don't look too deep through > the arguments). I guess you're right. If the c_* routine doesn't mind C++ specific trees and just cares about their types and pt.c then instantiates it, then it is fine. Jakub
On Thu, 27 Jun 2013, Jakub Jelinek wrote: > On Thu, Jun 27, 2013 at 05:54:10PM +0200, Marc Glisse wrote: >> I don't really see why, as I am still calling c_build_vec_perm_expr >> in the same cases, just possibly not exactly with the same arguments >> (they don't go through build_non_dependent_expr, but Jason seemed to >> imply that it did not matter since we don't look too deep through >> the arguments). > > I guess you're right. If the c_* routine doesn't mind C++ specific trees > and just cares about their types and pt.c then instantiates it, then it is > fine. Even if both are fine, if you prefer the long version (safer, or more uniform with the rest), please say so, I don't mind.
On Thu, 27 Jun 2013, Marc Glisse wrote: > On Thu, 27 Jun 2013, Jakub Jelinek wrote: > >> On Thu, Jun 27, 2013 at 05:54:10PM +0200, Marc Glisse wrote: >>> I don't really see why, as I am still calling c_build_vec_perm_expr >>> in the same cases, just possibly not exactly with the same arguments >>> (they don't go through build_non_dependent_expr, but Jason seemed to >>> imply that it did not matter since we don't look too deep through >>> the arguments). >> >> I guess you're right. If the c_* routine doesn't mind C++ specific trees >> and just cares about their types and pt.c then instantiates it, then it is >> fine. > > Even if both are fine, if you prefer the long version (safer, or more uniform > with the rest), please say so, I don't mind. I committed the short version, since that's the one I currently had in my tree (and I had tested it). We can always change it later. Thanks to you both for the comments,
Hi! On Thu, Jun 27, 2013 at 01:59:37PM +0200, Marc Glisse wrote: > --- testsuite/g++.dg/ext/pr57509.C (revision 0) > +++ testsuite/g++.dg/ext/pr57509.C (revision 0) > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > +/* { dg-options "-std=c++11" } */ > + > +template <bool> struct enable_if {}; > +template <> struct enable_if<true> {typedef void type;}; > +template <class T> void f (T& v) { v = __builtin_shuffle (v, v); } > +template <class T> void g (T) {} > +template <class T> auto g (T x) -> typename enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} > +typedef int v4i __attribute__((vector_size(4*sizeof(int)))); > +typedef float v4f __attribute__((vector_size(4*sizeof(float)))); > +int main(){ > + v4i a = {1,2,3,0}; > + f(a); > + v4f b = {1,2,3,0}; > + g(b); > +} Note this testcase fails on i686-linux: /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C: In function 'int main()': /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C:15:7: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C:15:7: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C:15:7: note: The ABI for passing parameters with 16-byte alignment has changed in GCC 4.6 The note is actually pruned, but the warnings aren't. -Wno-abi -Wno-psabi doesn't help, but -w does, so I'd suggest just to add -w to dg-options. Jakub
On Fri, 28 Jun 2013, Jakub Jelinek wrote: > Hi! > > On Thu, Jun 27, 2013 at 01:59:37PM +0200, Marc Glisse wrote: >> --- testsuite/g++.dg/ext/pr57509.C (revision 0) >> +++ testsuite/g++.dg/ext/pr57509.C (revision 0) >> @@ -0,0 +1,16 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-std=c++11" } */ >> + >> +template <bool> struct enable_if {}; >> +template <> struct enable_if<true> {typedef void type;}; >> +template <class T> void f (T& v) { v = __builtin_shuffle (v, v); } >> +template <class T> void g (T) {} >> +template <class T> auto g (T x) -> typename enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} >> +typedef int v4i __attribute__((vector_size(4*sizeof(int)))); >> +typedef float v4f __attribute__((vector_size(4*sizeof(float)))); >> +int main(){ >> + v4i a = {1,2,3,0}; >> + f(a); >> + v4f b = {1,2,3,0}; >> + g(b); >> +} > > Note this testcase fails on i686-linux: > /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C: In function 'int main()': > /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C:15:7: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] > /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C:15:7: warning: SSE vector argument without SSE enabled changes the ABI [enabled by default] > /usr/src/gcc/gcc/testsuite/g++.dg/ext/pr57509.C:15:7: note: The ABI for passing parameters with 16-byte alignment has changed in GCC 4.6 Gah, I know about this warning (that's why I pass vectors by pointer in most testcases) but somehow keep forgetting it :-( > The note is actually pruned, but the warnings aren't. -Wno-abi -Wno-psabi > doesn't help, I think it is a bug that the warning doesn't have an associated -Wxxx. Should it use -Wabi, -Wpsabi, or some new flag like -Wunsupported-vector? > but -w does, so I'd suggest just to add -w to dg-options. It seems better to do the following, so we still test for extra warnings: -template <class T> void g (T) {} -template <class T> auto g (T x) -> typename enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} +template <class T> void g (T const&) {} +template <class T> auto g (T const& x) -> typename enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} Ok?
On Fri, 28 Jun 2013, Marc Glisse wrote: > It seems better to do the following, so we still test for extra warnings: > > -template <class T> void g (T) {} > -template <class T> auto g (T x) -> typename > enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} > +template <class T> void g (T const&) {} > +template <class T> auto g (T const& x) -> typename > enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} I committed that as an obvious fix of my testcase. 2013-06-28 Marc Glisse <marc.glisse@inria.fr> PR c++/57509 * g++.dg/ext/pr57509.C: Pass vectors by reference to avoid warnings.
Index: cp/typeck.c =================================================================== --- cp/typeck.c (revision 200426) +++ cp/typeck.c (working copy) @@ -4864,20 +4864,48 @@ cp_build_binary_op (location_t location, if (final_type != 0) result = cp_convert (final_type, result, complain); if (TREE_OVERFLOW_P (result) && !TREE_OVERFLOW_P (op0) && !TREE_OVERFLOW_P (op1)) overflow_warning (location, result); return result; } + +/* Build a VEC_PERM_EXPR. + This is a simple wrapper for c_build_vec_perm_expr. */ +tree +cp_build_vec_perm_expr (location_t loc, + tree arg0, tree arg1, tree arg2, + tsubst_flags_t complain) +{ + tree expr; + tree orig_arg0 = arg0; + tree orig_arg1 = arg1; + tree orig_arg2 = arg2; + if (processing_template_decl) + { + if (type_dependent_expression_p (arg0) + || type_dependent_expression_p (arg1) + || type_dependent_expression_p (arg2)) + return build_min_nt_loc (loc, VEC_PERM_EXPR, arg0, arg1, arg2); + arg0 = build_non_dependent_expr (arg0); + arg1 = build_non_dependent_expr (arg1); + arg2 = build_non_dependent_expr (arg2); + } + expr = c_build_vec_perm_expr (loc, arg0, arg1, arg2, complain & tf_error); + if (processing_template_decl && expr != error_mark_node) + expr = build_min_non_dep (VEC_PERM_EXPR, expr, orig_arg0, orig_arg1, + orig_arg2); + return expr; +} /* Return a tree for the sum or difference (RESULTCODE says which) of pointer PTROP and integer INTOP. */ static tree cp_pointer_int_sum (enum tree_code resultcode, tree ptrop, tree intop) { tree res_type = TREE_TYPE (ptrop); /* pointer_int_sum() uses size_in_bytes() on the TREE_TYPE(res_type) Index: cp/pt.c =================================================================== --- cp/pt.c (revision 200426) +++ cp/pt.c (working copy) @@ -12457,20 +12457,21 @@ tsubst_copy (tree t, tree args, tsubst_f int i; for (i = 0; i < n; i++) TREE_OPERAND (t, i) = tsubst_copy (TREE_OPERAND (t, i), args, complain, in_decl); return result; } case COND_EXPR: case MODOP_EXPR: case PSEUDO_DTOR_EXPR: + case VEC_PERM_EXPR: { r = build_nt (code, tsubst_copy (TREE_OPERAND (t, 0), args, complain, in_decl), tsubst_copy (TREE_OPERAND (t, 1), args, complain, in_decl), tsubst_copy (TREE_OPERAND (t, 2), args, complain, in_decl)); TREE_NO_WARNING (r) = TREE_NO_WARNING (t); return r; } case NEW_EXPR: @@ -14621,20 +14622,27 @@ tsubst_copy_and_build (tree t, RETURN (r); } case TRANSACTION_EXPR: RETURN (tsubst_expr(t, args, complain, in_decl, integral_constant_expression_p)); case PAREN_EXPR: RETURN (finish_parenthesized_expr (RECUR (TREE_OPERAND (t, 0)))); + case VEC_PERM_EXPR: + RETURN (cp_build_vec_perm_expr (input_location, + RECUR (TREE_OPERAND (t, 0)), + RECUR (TREE_OPERAND (t, 1)), + RECUR (TREE_OPERAND (t, 2)), + complain)); + default: /* Handle Objective-C++ constructs, if appropriate. */ { tree subst = objcp_tsubst_copy_and_build (t, args, complain, in_decl, /*function_p=*/false); if (subst) RETURN (subst); } RETURN (tsubst_copy (t, args, complain, in_decl)); Index: cp/parser.c =================================================================== --- cp/parser.c (revision 200426) +++ cp/parser.c (working copy) @@ -5684,23 +5684,25 @@ cp_parser_postfix_expression (cp_parser vec = cp_parser_parenthesized_expression_list (parser, non_attr, /*cast_p=*/false, /*allow_expansion_p=*/true, /*non_constant_p=*/NULL); if (vec == NULL) return error_mark_node; FOR_EACH_VEC_ELT (*vec, i, p) mark_exp_read (p); if (vec->length () == 2) - return c_build_vec_perm_expr (loc, (*vec)[0], NULL_TREE, (*vec)[1]); + return cp_build_vec_perm_expr (loc, (*vec)[0], NULL_TREE, (*vec)[1], + tf_warning_or_error); else if (vec->length () == 3) - return c_build_vec_perm_expr (loc, (*vec)[0], (*vec)[1], (*vec)[2]); + return cp_build_vec_perm_expr (loc, (*vec)[0], (*vec)[1], (*vec)[2], + tf_warning_or_error); else { error_at (loc, "wrong number of arguments to " "%<__builtin_shuffle%>"); return error_mark_node; } break; } default: Index: cp/cp-tree.h =================================================================== --- cp/cp-tree.h (revision 200426) +++ cp/cp-tree.h (working copy) @@ -6035,20 +6035,23 @@ extern tree type_after_usual_arithmetic_ extern tree common_pointer_type (tree, tree); extern tree composite_pointer_type (tree, tree, tree, tree, composite_pointer_operation, tsubst_flags_t); extern tree merge_types (tree, tree); extern tree strip_array_domain (tree); extern tree check_return_expr (tree, bool *); extern tree cp_build_binary_op (location_t, enum tree_code, tree, tree, tsubst_flags_t); +extern tree cp_build_vec_perm_expr (location_t, + tree, tree, tree, + tsubst_flags_t); #define cxx_sizeof(T) cxx_sizeof_or_alignof_type (T, SIZEOF_EXPR, true) extern tree build_simple_component_ref (tree, tree); extern tree build_ptrmemfunc_access_expr (tree, tree); extern tree build_address (tree); extern tree build_typed_address (tree, tree); extern tree build_nop (tree, tree); extern tree non_reference (tree); extern tree lookup_anon_field (tree, tree); extern bool invalid_nonstatic_memfn_p (tree, tsubst_flags_t); extern tree convert_member_func_to_ptr (tree, tree, tsubst_flags_t); Index: testsuite/g++.dg/ext/pr57509.C =================================================================== --- testsuite/g++.dg/ext/pr57509.C (revision 0) +++ testsuite/g++.dg/ext/pr57509.C (revision 0) @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-options "-std=c++11" } */ + +template <bool> struct enable_if {}; +template <> struct enable_if<true> {typedef void type;}; +template <class T> void f (T& v) { v = __builtin_shuffle (v, v); } +template <class T> void g (T) {} +template <class T> auto g (T x) -> typename enable_if<sizeof(__builtin_shuffle(x,x))!=2>::type {} +typedef int v4i __attribute__((vector_size(4*sizeof(int)))); +typedef float v4f __attribute__((vector_size(4*sizeof(float)))); +int main(){ + v4i a = {1,2,3,0}; + f(a); + v4f b = {1,2,3,0}; + g(b); +} Property changes on: testsuite/g++.dg/ext/pr57509.C ___________________________________________________________________ Added: svn:eol-style + native Added: svn:keywords + Author Date Id Revision URL Index: c-family/c-common.c =================================================================== --- c-family/c-common.c (revision 200426) +++ c-family/c-common.c (working copy) @@ -2253,95 +2253,103 @@ vector_types_convertible_p (const_tree t and have vector types, V0 has the same type as V1, and the number of elements of V0, V1, MASK is the same. In case V1 is a NULL_TREE it is assumed that __builtin_shuffle was called with two arguments. In this case implementation passes the first argument twice in order to share the same tree code. This fact could enable the mask-values being twice the vector length. This is an implementation accident and this semantics is not guaranteed to the user. */ tree -c_build_vec_perm_expr (location_t loc, tree v0, tree v1, tree mask) +c_build_vec_perm_expr (location_t loc, tree v0, tree v1, tree mask, + bool complain) { tree ret; bool wrap = true; bool maybe_const = false; bool two_arguments = false; if (v1 == NULL_TREE) { two_arguments = true; v1 = v0; } if (v0 == error_mark_node || v1 == error_mark_node || mask == error_mark_node) return error_mark_node; if (TREE_CODE (TREE_TYPE (mask)) != VECTOR_TYPE || TREE_CODE (TREE_TYPE (TREE_TYPE (mask))) != INTEGER_TYPE) { - error_at (loc, "__builtin_shuffle last argument must " - "be an integer vector"); + if (complain) + error_at (loc, "__builtin_shuffle last argument must " + "be an integer vector"); return error_mark_node; } if (TREE_CODE (TREE_TYPE (v0)) != VECTOR_TYPE || TREE_CODE (TREE_TYPE (v1)) != VECTOR_TYPE) { - error_at (loc, "__builtin_shuffle arguments must be vectors"); + if (complain) + error_at (loc, "__builtin_shuffle arguments must be vectors"); return error_mark_node; } if (TYPE_MAIN_VARIANT (TREE_TYPE (v0)) != TYPE_MAIN_VARIANT (TREE_TYPE (v1))) { - error_at (loc, "__builtin_shuffle argument vectors must be of " - "the same type"); + if (complain) + error_at (loc, "__builtin_shuffle argument vectors must be of " + "the same type"); return error_mark_node; } if (TYPE_VECTOR_SUBPARTS (TREE_TYPE (v0)) != TYPE_VECTOR_SUBPARTS (TREE_TYPE (mask)) && TYPE_VECTOR_SUBPARTS (TREE_TYPE (v1)) != TYPE_VECTOR_SUBPARTS (TREE_TYPE (mask))) { - error_at (loc, "__builtin_shuffle number of elements of the " - "argument vector(s) and the mask vector should " - "be the same"); + if (complain) + error_at (loc, "__builtin_shuffle number of elements of the " + "argument vector(s) and the mask vector should " + "be the same"); return error_mark_node; } if (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (TREE_TYPE (v0)))) != GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (TREE_TYPE (mask))))) { - error_at (loc, "__builtin_shuffle argument vector(s) inner type " - "must have the same size as inner type of the mask"); + if (complain) + error_at (loc, "__builtin_shuffle argument vector(s) inner type " + "must have the same size as inner type of the mask"); return error_mark_node; } if (!c_dialect_cxx ()) { /* Avoid C_MAYBE_CONST_EXPRs inside VEC_PERM_EXPR. */ v0 = c_fully_fold (v0, false, &maybe_const); wrap &= maybe_const; if (two_arguments) v1 = v0 = save_expr (v0); else { v1 = c_fully_fold (v1, false, &maybe_const); wrap &= maybe_const; } mask = c_fully_fold (mask, false, &maybe_const); wrap &= maybe_const; } + else if (two_arguments) + v1 = v0 = save_expr (v0); ret = build3_loc (loc, VEC_PERM_EXPR, TREE_TYPE (v0), v0, v1, mask); if (!c_dialect_cxx () && !wrap) ret = c_wrap_maybe_const (ret, true); return ret; } /* Like tree.c:get_narrower, but retain conversion from C++0x scoped enum Index: c-family/c-common.h =================================================================== --- c-family/c-common.h (revision 200426) +++ c-family/c-common.h (working copy) @@ -904,21 +904,21 @@ extern tree resolve_overloaded_builtin ( extern tree finish_label_address_expr (tree, location_t); /* Same function prototype, but the C and C++ front ends have different implementations. Used in c-common.c. */ extern tree lookup_label (tree); extern tree lookup_name (tree); extern bool lvalue_p (const_tree); extern bool vector_targets_convertible_p (const_tree t1, const_tree t2); extern bool vector_types_convertible_p (const_tree t1, const_tree t2, bool emit_lax_note); -extern tree c_build_vec_perm_expr (location_t, tree, tree, tree); +extern tree c_build_vec_perm_expr (location_t, tree, tree, tree, bool = true); extern rtx c_expand_expr (tree, rtx, enum machine_mode, int, rtx *); extern void init_c_lex (void); extern void c_cpp_builtins (cpp_reader *); extern void c_cpp_builtins_optimize_pragma (cpp_reader *, tree, tree); extern bool c_cpp_error (cpp_reader *, int, int, location_t, unsigned int, const char *, va_list *) ATTRIBUTE_GCC_DIAG(6,0);