[Skiboot] [PATCH 1/5] simplify GET/SETFIELD() use, add MASK_TO_LSH()

[Dan Streetman]

On Tue, Feb 17, 2015 at 4:09 PM, Dan Streetman <ddstreet at us.ibm.com> wrote:
> On Tue, Feb 17, 2015 at 3:44 PM, Benjamin Herrenschmidt
> <benh at kernel.crashing.org> wrote:
>> On Tue, 2015-02-17 at 15:38 -0500, Dan Streetman wrote:
>>> Currently, the GETFIELD() and SETFIELD() macros require the user to define
>>> both the _MASK and the _LSH of the field. However, the shift of the field
>>> is trivially determinable and there is no reason it needs to be specified
>>> by the user.
>>>
>>> Add MASK_TO_LSH() macro, which determines the shift of a given mask.
>>>
>>> Change GETFIELD() and SETFIELD() to use MASK_TO_LSH() to determine the
>>> shift of the specified mask, instead of requiring it to be specified by
>>> the user.
>>
>> This is quite nice, did you check gcc was resolving it properly at
>> compile time ?
>
> Yep, gcc compiles it fine

To clarify - I did not check the binary output for correctness, I only
booted the resulting skiboot.lid, which worked as expected.

> and I booted the resulting skiboot.lid fine
> also.  I'm pretty sure that all gcc versions implement
> __builtin_ffsl()...probably any other compiler would too, although I
> assume we really only plan on gcc.
>
>>
>> We could get rid of a whole pile of our _LSH definitions all over the
>> place...
>
> yep, next patch in the set does that, and removes a lot of the _MASK suffixes.
>
>>
>>> Signed-off-by: Dan Streetman <ddstreet at ieee.org>
>>> ---
>>>  include/bitutils.h | 11 ++++++-----
>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/bitutils.h b/include/bitutils.h
>>> index 7e5b6eb..baa752b 100644
>>> --- a/include/bitutils.h
>>> +++ b/include/bitutils.h
>>> @@ -36,15 +36,16 @@
>>>   * PPC bitmask field manipulation
>>>   */
>>>
>>> +/* Find left shift from first set bit in mask */
>>> +#define MASK_TO_LSH(m)               (__builtin_ffsl(m) - 1)
>>> +
>>>  /* Extract field fname from val */
>>> -#define GETFIELD(fname, val)                 \
>>> -     (((val) & fname##_MASK) >> fname##_LSH)
>>> +#define GETFIELD(m, v)               (((v) & (m)) >> MASK_TO_LSH(m))
>>>
>>>  /* Set field fname of oval to fval
>>>   * NOTE: oval isn't modified, the combined result is returned
>>>   */
>>> -#define SETFIELD(fname, oval, fval)                  \
>>> -     (((oval) & ~fname##_MASK) | \
>>> -      ((((typeof(oval))(fval)) << fname##_LSH) & fname##_MASK))
>>> +#define SETFIELD(m, v, val)                          \
>>> +     (((v) & ~(m)) | ((((typeof(v))(val)) << MASK_TO_LSH(m)) & (m)))
>>>
>>>  #endif /* __BITUTILS_H */
>>
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot