| Message ID | 1424205534-25180-2-git-send-email-ddstreet@ieee.org |
|---|---|
| State | Accepted |
| Headers | show |
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 ? We could get rid of a whole pile of our _LSH definitions all over the place... > Signed-off-by: Dan Streetman <ddstreet@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 */
On Tue, Feb 17, 2015 at 3:44 PM, Benjamin Herrenschmidt <benh@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 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@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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Tue, Feb 17, 2015 at 4:09 PM, Dan Streetman <ddstreet@us.ibm.com> wrote: > On Tue, Feb 17, 2015 at 3:44 PM, Benjamin Herrenschmidt > <benh@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@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@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/skiboot
On Tue, Feb 17, 2015 at 4:20 PM, Dan Streetman <ddstreet@us.ibm.com> wrote: > On Tue, Feb 17, 2015 at 4:09 PM, Dan Streetman <ddstreet@us.ibm.com> wrote: >> On Tue, Feb 17, 2015 at 3:44 PM, Benjamin Herrenschmidt >> <benh@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. Re: gcc fully resolving the value, I tried to look at the ppc asm, but my ppc asm isn't good enough to tell what gcc's doing. Here is an example though, in case you can tell (taken with objdump -S): ci = instance; cfg = SETFIELD(NX_P8_842_CFG_CI, cfg, ci << NX_P8_842_CFG_CI_LSHIFT); 2c4: 78 a5 13 40 rldicl r5,r5,2,13 2c8: 78 a5 f0 02 rotldi r5,r5,62 2cc: 64 a5 f8 00 oris r5,r5,63488 2d0: 60 a5 00 01 ori r5,r5,1 /* Enable all functions */ cfg = SETFIELD(NX_P8_842_CFG_FC_ENABLE, cfg, CFG_842_FC_ENABLE); cfg = SETFIELD(NX_P8_842_CFG_ENABLE, cfg, CFG_842_ENABLE); 2d4: 7c a5 4b 78 or r5,r5,r9 2d8: f8 a1 00 70 std r5,112(r1) rc = xscom_write(gcid, xcfg, cfg); 2dc: 48 00 00 01 bl 2dc <.nx_create_842_node+0x2dc> 2e0: 60 00 00 00 nop if (rc) 2e4: 7c 7b 1b 79 mr. r27,r3 2e8: 41 82 00 30 beq 318 <.nx_create_842_node+0x318> prerror("NX%d: ERROR: 842 CT %u CI %u config failure %d\n", 2ec: 3c 82 00 00 addis r4,r2,0 2f0: 38 60 00 03 li r3,3 2f4: 38 84 00 00 addi r4,r4,0 2f8: 7f e5 fb 78 mr r5,r31 2fc: 38 c0 00 03 li r6,3 > >> 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@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@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/skiboot
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 */
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. Signed-off-by: Dan Streetman <ddstreet@ieee.org> --- include/bitutils.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)