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

[Victor Do Nascimento <Victor.DoNascimento@arm.com>]

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.

Cheers,
Victor