Re: [PATCH 6/6] aarch64: Add front-end argument type checking for target builtins

[Richard Sandiford <richard.sandiford@arm.com>]

"Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> writes:
> On 09/10/2023 14:12, Victor Do Nascimento wrote:
>> 
>> 
>> On 10/7/23 12:53, Richard Sandiford wrote:
>>> Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
>>>> On 03/10/2023 16:18, Victor Do Nascimento wrote:
>>>>> In implementing the ACLE read/write system register builtins it was
>>>>> observed that leaving argument type checking to be done at expand-time
>>>>> meant that poorly-formed function calls were being "fixed" by certain
>>>>> optimization passes, meaning bad code wasn't being properly picked up
>>>>> in checking.
>>>>>
>>>>> Example:
>>>>>
>>>>>     const char *regname = "amcgcr_el0";
>>>>>     long long a = __builtin_aarch64_rsr64 (regname);
>>>>>
>>>>> is reduced by the ccp1 pass to
>>>>>
>>>>>     long long a = __builtin_aarch64_rsr64 ("amcgcr_el0");
>>>>>
>>>>> As these functions require an argument of STRING_CST type, there needs
>>>>> to be a check carried out by the front-end capable of picking this up.
>>>>>
>>>>> The introduced `check_general_builtin_call' function will be called by
>>>>> the TARGET_CHECK_BUILTIN_CALL hook whenever a call to a builtin
>>>>> belonging to the AARCH64_BUILTIN_GENERAL category is encountered,
>>>>> carrying out any appropriate checks associated with a particular
>>>>> builtin function code.
>>>>
>>>> Doesn't this prevent reasonable wrapping of the __builtin... names with
>>>> something more palatable?  Eg:
>>>>
>>>> static inline __attribute__(("always_inline")) long long get_sysreg_ll
>>>> (const char *regname)
>>>> {
>>>>     return __builtin_aarch64_rsr64 (regname);
>>>> }
>>>>
>>>> ...
>>>>     long long x = get_sysreg_ll("amcgcr_el0");
>>>> ...
>>>
>>> I think it's case of picking your poison.  If we didn't do this,
>>> and only checked later, then it's unlikely that GCC and Clang would
>>> be consistent about when a constant gets folded soon enough.
>>>
>>> But yeah, it means that the above would need to be a macro in C.
>>> Enlightened souls using C++ could instead do:
>>>
>>>    template<const char *regname>
>>>    long long get_sysreg_ll()
>>>    {
>>>      return __builtin_aarch64_rsr64(regname);
>>>    }
>>>
>>>    ... get_sysreg_ll<"amcgcr_el0">() ...
>>>
>>> Or at least I hope so.  Might be nice to have a test for this.
>>>
>>> Thanks,
>>> Richard
>> 
>> As Richard Earnshaw mentioned, this does break the use of `static inline __attribute__(("always_inline"))', something I had found out in my testing.  My chosen implementation was indeed, to quote Richard Sandiford, a case of "picking your poison" to have things line up with Clang and behaving consistently across optimization levels.
>> 
>> Relaxing the the use of `TARGET_CHECK_BUILTIN_CALL' meant optimizations were letting too many things through. Example:
>> 
>> const char *regname = "amcgcr_el0";
>> long long a = __builtin_aarch64_rsr64 (regname);
>> 
>> gets folded to
>> 
>> long long a = __builtin_aarch64_rsr64 ("amcgcr_el0");
>> 
>> and compilation passes at -01 even though it fails at -O0.
>> 
>> I had, however, not given any thought to the use of a template as a valid C++ alternative.
>> 
>> I will evaluate the use of templates and add tests accordingly.
>
> This just seems inconsistent with all the builtins we already have that require literal constants for parameters.  For example (to pick just one of many), vshr_n_q8(), where the second parameter must be a literal value.  In practice we accept anything that resolves to a compile-time constant integer expression and rely on that to avoid having to have hundreds of macros binding the ACLE names to the underlying builtin equivalents.

That's true for the way that GCC handles things like Advanced SIMD.
But Clang behaves differently.  So does GCC's SVE ACLE implementation.
Both of those (try to) follow the language rules about what is a constant
expression.

Thanks,
Richard