| Message ID | 20240711214305.3193022-5-christophe.lyon@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [01/15] arm: [MVE intrinsics] improve comment for orrq shape | expand |
On 11/07/2024 22:42, Christophe Lyon wrote: > + bool > + check (function_checker &c) const override > + { > + if (c.mode_suffix_id == MODE_none) > + return true; > + > + unsigned int bits = c.type_suffix (0).element_bits; > + return c.require_immediate_range (1, 1, bits); > + } When trying to understand how this worked I bumped into the following: If you pass a value that's not in the appropriate range like 33, you'll see: vcvt.c:5:10: error: passing 33 to argument 2 of 'vcvtq_n', which expects a value in the range [1, 32] If you however pick a negative number like -3? You get: vcvt.c:5:10: error: argument 2 of 'vcvtq_n' must be an integer constant expression This is somewhat confusing and yes we could change the message to say 'must be a positive integer constant expression', which might be clear enough in this case, but less so if you do something like 1<<7 for this intrinsic, because the signature looks like: > + build_all (b, "v0,v1,ss8", group, MODE_n, preserve_user_namespace); which converts the immediate to a signed 8-bit value, making 1<<7 a negative number, and the intrinsic specs defines the parameter as a signed 32-bit integer. So if a user accidentally passes 1<<7 here, they will get: vcvt.c:5:10: error: argument 2 of 'vcvtq_n' must be an integer constant expression Potentially making users think GCC does not support inline evaluation for these parameters, which would be sad. So we should at least change the signature to be "v0,v1,ss32", but I suggest we deviate and make the last parameter in the signature either a su32 or a su64. As the framework code expects this value to fit into a uhwi, which I suspect is because SVE always defines the parameters as uint64_t constants, i.e. svxar_n_s64, disclaimer I didn't check all SVE intrinsics. Initially I was going to suggest fold_converting the arg in 'function_checker::require_immediate' into a long_long_unsigned_type_node, which also does what we want in these examples, but perhaps going the signature way is cleaner and more inline with the framework as is. We probably want to keep it as much as SVE as possible, so we can 'borrow' code more easily if the need arises. @Richard S: I added you to this review as you know the SVE intrinsic code better than most (if not all). So hopefully you can let us know if you have an opinion on this. Kind Regards, Andre
"Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes: > On 11/07/2024 22:42, Christophe Lyon wrote: >> + bool >> + check (function_checker &c) const override >> + { >> + if (c.mode_suffix_id == MODE_none) >> + return true; >> + >> + unsigned int bits = c.type_suffix (0).element_bits; >> + return c.require_immediate_range (1, 1, bits); >> + } > > When trying to understand how this worked I bumped into the following: > > If you pass a value that's not in the appropriate range like 33, you'll see: > > vcvt.c:5:10: error: passing 33 to argument 2 of 'vcvtq_n', which expects > a value in the range [1, 32] > > If you however pick a negative number like -3? You get: > vcvt.c:5:10: error: argument 2 of 'vcvtq_n' must be an integer constant > expression > > This is somewhat confusing and yes we could change the message to say > 'must be a positive integer constant expression', which might be clear > enough in this case, but less so if you do something like 1<<7 for this > intrinsic, because the signature looks like: > > > + build_all (b, "v0,v1,ss8", group, MODE_n, preserve_user_namespace); > > which converts the immediate to a signed 8-bit value, making 1<<7 a > negative number, and the intrinsic specs defines the parameter as a > signed 32-bit integer. So if a user accidentally passes 1<<7 here, they > will get: > vcvt.c:5:10: error: argument 2 of 'vcvtq_n' must be an integer constant > expression > > Potentially making users think GCC does not support inline evaluation > for these parameters, which would be sad. > > So we should at least change the signature to be "v0,v1,ss32", but I > suggest we deviate and make the last parameter in the signature either a > su32 or a su64. As the framework code expects this value to fit into a > uhwi, which I suspect is because SVE always defines the parameters as > uint64_t constants, i.e. svxar_n_s64, disclaimer I didn't check all SVE > intrinsics. > > Initially I was going to suggest fold_converting the arg in > 'function_checker::require_immediate' into a > long_long_unsigned_type_node, which also does what we want in these > examples, but perhaps going the signature way is cleaner and more inline > with the framework as is. We probably want to keep it as much as SVE as > possible, so we can 'borrow' code more easily if the need arises. > > @Richard S: I added you to this review as you know the SVE intrinsic > code better than most (if not all). So hopefully you can let us know if > you have an opinion on this. Sorry for the slow reply, but yeah, I agree with the suggestion. One of the reasons for using uint64_t for SVE was to prevent constants being trimmed by argument type coercion before the target gets to see them. E.g. passing 1ULL<<32 to an svuint32_t would be truncated to 0 before the target sees it. Obviously that doesn't help with 128-bit constants or BitInts (the latter of which weren't a thing when this was added) but those are much rarer. The SVE error message prints constants as signed, so even though -1 gets converted to the maximum uint64_t, it should still be printed as -1. Thanks, Richard
diff --git a/gcc/config/arm/arm-mve-builtins-shapes.cc b/gcc/config/arm/arm-mve-builtins-shapes.cc index 0520a8331db..e1c5dd2c0f2 100644 --- a/gcc/config/arm/arm-mve-builtins-shapes.cc +++ b/gcc/config/arm/arm-mve-builtins-shapes.cc @@ -330,7 +330,8 @@ build_16_32 (function_builder &b, const char *signature, struct nonoverloaded_base : public function_shape { bool - explicit_type_suffix_p (unsigned int, enum predication_index, enum mode_suffix_index) const override + explicit_type_suffix_p (unsigned int, enum predication_index, + enum mode_suffix_index, type_suffix_info) const override { return true; } @@ -360,7 +361,8 @@ template<unsigned int EXPLICIT_MASK> struct overloaded_base : public function_shape { bool - explicit_type_suffix_p (unsigned int i, enum predication_index, enum mode_suffix_index) const override + explicit_type_suffix_p (unsigned int i, enum predication_index, + enum mode_suffix_index, type_suffix_info) const override { return (EXPLICIT_MASK >> i) & 1; } @@ -1856,7 +1858,7 @@ struct unary_n_def : public overloaded_base<0> { bool explicit_type_suffix_p (unsigned int, enum predication_index pred, - enum mode_suffix_index) const override + enum mode_suffix_index, type_suffix_info) const override { return pred != PRED_m; } @@ -1979,6 +1981,106 @@ struct unary_widen_acc_def : public overloaded_base<0> }; SHAPE (unary_widen_acc) +/* <T0>_t foo_t0[_t1](<T1>_t) + <T0>_t foo_t0_n[_t1](<T1>_t, const int) + + Example: vcvtq. + float32x4_t [__arm_]vcvtq[_f32_s32](int32x4_t a) + float32x4_t [__arm_]vcvtq_m[_f32_s32](float32x4_t inactive, int32x4_t a, mve_pred16_t p) + float32x4_t [__arm_]vcvtq_x[_f32_s32](int32x4_t a, mve_pred16_t p) + float32x4_t [__arm_]vcvtq_n[_f32_s32](int32x4_t a, const int imm6) + float32x4_t [__arm_]vcvtq_m_n[_f32_s32](float32x4_t inactive, int32x4_t a, const int imm6, mve_pred16_t p) + float32x4_t [__arm_]vcvtq_x_n[_f32_s32](int32x4_t a, const int imm6, mve_pred16_t p) + int32x4_t [__arm_]vcvtq_s32_f32(float32x4_t a) + int32x4_t [__arm_]vcvtq_m[_s32_f32](int32x4_t inactive, float32x4_t a, mve_pred16_t p) + int32x4_t [__arm_]vcvtq_x_s32_f32(float32x4_t a, mve_pred16_t p) + int32x4_t [__arm_]vcvtq_n_s32_f32(float32x4_t a, const int imm6) + int32x4_t [__arm_]vcvtq_m_n[_s32_f32](int32x4_t inactive, float32x4_t a, const int imm6, mve_pred16_t p) + int32x4_t [__arm_]vcvtq_x_n_s32_f32(float32x4_t a, const int imm6, mve_pred16_t p) */ +struct vcvt_def : public overloaded_base<0> +{ + bool + explicit_type_suffix_p (unsigned int i, enum predication_index pred, + enum mode_suffix_index, + type_suffix_info type_info) const override + { + if (pred != PRED_m + && ((i == 0 && type_info.integer_p) + || (i == 1 && type_info.float_p))) + return true; + return false; + } + + bool + explicit_mode_suffix_p (enum predication_index, + enum mode_suffix_index) const override + { + return true; + } + + void + build (function_builder &b, const function_group_info &group, + bool preserve_user_namespace) const override + { + b.add_overloaded_functions (group, MODE_none, preserve_user_namespace); + b.add_overloaded_functions (group, MODE_n, preserve_user_namespace); + build_all (b, "v0,v1", group, MODE_none, preserve_user_namespace); + build_all (b, "v0,v1,ss8", group, MODE_n, preserve_user_namespace); + } + + tree + resolve (function_resolver &r) const override + { + unsigned int i, nargs; + type_suffix_index from_type; + tree res; + unsigned int nimm = (r.mode_suffix_id == MODE_none) ? 0 : 1; + + if (!r.check_gp_argument (1 + nimm, i, nargs) + || (from_type + = r.infer_vector_type (i - nimm)) == NUM_TYPE_SUFFIXES) + return error_mark_node; + + if (nimm > 0 + && !r.require_integer_immediate (i)) + return error_mark_node; + + type_suffix_index to_type; + + if (type_suffixes[from_type].integer_p) + { + to_type = find_type_suffix (TYPE_float, + type_suffixes[from_type].element_bits); + } + else + { + /* This should not happen: when 'from_type' is float, the type + suffixes are not overloaded (except for "m" predication, + handled above). */ + gcc_assert (r.pred == PRED_m); + + /* Get the return type from the 'inactive' argument. */ + to_type = r.infer_vector_type (0); + } + + if ((res = r.lookup_form (r.mode_suffix_id, to_type, from_type))) + return res; + + return r.report_no_such_form (from_type); + } + + bool + check (function_checker &c) const override + { + if (c.mode_suffix_id == MODE_none) + return true; + + unsigned int bits = c.type_suffix (0).element_bits; + return c.require_immediate_range (1, 1, bits); + } +}; +SHAPE (vcvt) + /* <T0>_t vfoo[_t0](<T0>_t, <T0>_t, mve_pred16_t) i.e. a version of the standard ternary shape in which diff --git a/gcc/config/arm/arm-mve-builtins-shapes.h b/gcc/config/arm/arm-mve-builtins-shapes.h index 61aa4fa73b3..9a112ceeb29 100644 --- a/gcc/config/arm/arm-mve-builtins-shapes.h +++ b/gcc/config/arm/arm-mve-builtins-shapes.h @@ -77,6 +77,7 @@ namespace arm_mve extern const function_shape *const unary_n; extern const function_shape *const unary_widen; extern const function_shape *const unary_widen_acc; + extern const function_shape *const vcvt; extern const function_shape *const vpsel; } /* end namespace arm_mve::shapes */ diff --git a/gcc/config/arm/arm-mve-builtins.cc b/gcc/config/arm/arm-mve-builtins.cc index 7e8217666fe..ea44f463dd8 100644 --- a/gcc/config/arm/arm-mve-builtins.cc +++ b/gcc/config/arm/arm-mve-builtins.cc @@ -823,7 +823,8 @@ function_builder::get_name (const function_instance &instance, for (unsigned int i = 0; i < 2; ++i) if (!overloaded_p || instance.shape->explicit_type_suffix_p (i, instance.pred, - instance.mode_suffix_id)) + instance.mode_suffix_id, + instance.type_suffix (i))) append_name (instance.type_suffix (i).string); return finish_name (); } @@ -1001,9 +1002,11 @@ function_builder::add_overloaded_functions (const function_group_info &group, for (unsigned int pi = 0; group.preds[pi] != NUM_PREDS; ++pi) { unsigned int explicit_type0 - = (*group.shape)->explicit_type_suffix_p (0, group.preds[pi], mode); + = (*group.shape)->explicit_type_suffix_p (0, group.preds[pi], mode, + type_suffixes[NUM_TYPE_SUFFIXES]); unsigned int explicit_type1 - = (*group.shape)->explicit_type_suffix_p (1, group.preds[pi], mode); + = (*group.shape)->explicit_type_suffix_p (1, group.preds[pi], mode, + type_suffixes[NUM_TYPE_SUFFIXES]); if ((*group.shape)->skip_overload_p (group.preds[pi], mode)) continue; diff --git a/gcc/config/arm/arm-mve-builtins.h b/gcc/config/arm/arm-mve-builtins.h index f282236a843..3306736bff0 100644 --- a/gcc/config/arm/arm-mve-builtins.h +++ b/gcc/config/arm/arm-mve-builtins.h @@ -571,9 +571,13 @@ public: class function_shape { public: - virtual bool explicit_type_suffix_p (unsigned int, enum predication_index, enum mode_suffix_index) const = 0; - virtual bool explicit_mode_suffix_p (enum predication_index, enum mode_suffix_index) const = 0; - virtual bool skip_overload_p (enum predication_index, enum mode_suffix_index) const = 0; + virtual bool explicit_type_suffix_p (unsigned int, enum predication_index, + enum mode_suffix_index, + type_suffix_info) const = 0; + virtual bool explicit_mode_suffix_p (enum predication_index, + enum mode_suffix_index) const = 0; + virtual bool skip_overload_p (enum predication_index, + enum mode_suffix_index) const = 0; /* Define all functions associated with the given group. */ virtual void build (function_builder &,