Re: [PATCH v3 26/60] target/arm: Replace sentinels with ARRAY_SIZE in cpregs.h

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 4/22/22 02:37, Peter Maydell wrote:
>> On Sun, 17 Apr 2022 at 19:08, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> Remove a possible source of error by removing REGINFO_SENTINEL
>>> and using ARRAY_SIZE (convinently hidden inside a macro) to
>>> find the end of the set of regs being registered or modified.
>>>
>>> The space saved by not having the extra array element reduces
>>> the executable's .data.rel.ro section by about 9k.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>> 
>>> +#define define_arm_cp_regs_with_opaque(CPU, REGS, OPAQUE)               \
>>> +    do {                                                                \
>>> +        QEMU_BUILD_BUG_ON(ARRAY_SIZE(REGS) == 0);                       \
>>> +        if (ARRAY_SIZE(REGS) == 1) {                                    \
>>> +            define_one_arm_cp_reg_with_opaque(CPU, REGS, OPAQUE);       \
>>> +        } else {                                                        \
>>> +            define_arm_cp_regs_with_opaque_len(CPU, REGS, OPAQUE,       \
>>> +                                               ARRAY_SIZE(REGS));       \
>>> +        }                                                               \
>>> +    } while (0)
>> Do we actually need to special case "array has one element" here,
>> or is this just efficiency?
>> Anyway
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Just efficiency.  There seem to be a lot of these.

If you moved define_arm_cp_regs_with_opaque_len into the header as an
inline surely the compiler could figure it out when presented with a
constant i? The would avoid the need for the special casing in the macro
expansion right?

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>
>
> r~


-- 
Alex Bennée