"Andre Vieira (lists)" <andre.simoesdiasvie...@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
