Well, I *think* having not extensively surveyed the code, that the vast majority of the time the bits that are being extracted are constant, so the templated form would be the default, and you'd just need to use the current form in the few cases where it's not fixed. As somebody suggested, we could use a macro to make it look a little more uniform and keep the arguments in the same order. Perhaps:
#define bits(val, first, last) _bits_template<first, last>(val) and call the full flexibility function bitsv (for variable). Most of the time you'd use the normal version, except when it yells at you and forces you to use bitsv. This is not ideal in I think three ways. 1. Macros are not namespaced. MyFavoritClass::bits would be problematic but is totally fine today. 2. Slightly more complexity than today. The complexity is *mostly* obscured, which can be a good or bad thing depending on circumstances. 3. If somebody does copy paste coding and doesn't know *why* something is bitsv vs bits, they're more likely to use the wrong one than if they were starting from scratch. Importantly, if somebody *does* use the wrong thing, the only way they could do that would be to use the variable form unnecessarily. The other way around would definitely be a compiler error. If they do that, then we would be no worse off than we are today, we would just lose a little bit of the error checking which we have none of now. I don't think this is a slam dunk obvious right thing to do, but I think it has some merits and is worth considering. If in the future somebody does either figure out some crazy clever way to do this without the templates, or the standard evolves to the point where we can use something more elegant, then it should be relatively easy to move to that instead. Gabe On Fri, Sep 18, 2020 at 8:35 AM Jason Lowe-Power <ja...@lowepower.com> wrote: > Sorry for the spam... > > One last thing: We have to keep both bits(val, first, last) *and* > bits<first, last>(val) because sometimes first and last are *not* > constexpr. If they were *always* constexpr, this would be much simpler (I > think). > > Cheers, > Jason > > On Fri, Sep 18, 2020 at 8:33 AM Jason Lowe-Power <ja...@lowepower.com> > wrote: > >> I don't like the following change >> >> bits(val, first, last) >> >> would now be >> >> bits<first,last>(val). >> >> IMO, it's confusing to put function *parameters* as template arguments. >> >> This would mean that when you use the bits() function, you'll have to >> think about "are these constexpr that I'm using, and, if so, should I use >> bits() or bits<>()?" >> >> We can go through and manually change the current code, but for new code, >> this will be yet another thing that we'd have to catch during code review. >> >> Just my two cents. I won't block anything, I just think that readability >> is more important than a little* performance. >> >> *It depends on how much performance difference assert() vs >> static_assert() is in this case. >> >> Cheers, >> Jason >> >> On Fri, Sep 18, 2020 at 8:29 AM Gutierrez, Anthony < >> anthony.gutier...@amd.com> wrote: >> >>> [AMD Public Use] >>> >>> >>> >>> Hey, Jason perhaps you mentioned this somewhere but what is the reason >>> for such a strong aversion to the template approach? It seems to solve the >>> issue nicely with what seems to be a minor change in syntax. gem5 is C++, >>> so we should allow users to write C++. >>> >>> >>> >>> Tony >>> >>> >>> >>> *From:* Jason Lowe-Power via gem5-dev <gem5-dev@gem5.org> >>> *Sent:* Friday, September 18, 2020 8:05 AM >>> *To:* Gabe Black <gabebl...@google.com> >>> *Cc:* gem5 Developer List <gem5-dev@gem5.org>; Jason Lowe-Power < >>> ja...@lowepower.com> >>> *Subject:* [gem5-dev] Re: A few quick thoughts >>> >>> >>> >>> [CAUTION: External Email] >>> >>> There is another option to keep the function-like syntax, but get the >>> constexpr via templates: A preprocessor macro: >>> >>> >>> >>> #define bits(val, first, last) bits<first,last>(val) >>> >>> >>> >>> The major downside is that we can't overload preprocessor macros. We'd >>> have to have two name bits_const() and bits(), which I also don't like. >>> >>> >>> >>> Personally, I strongly don't want to lose the function-like syntax for >>> the bits function. I won't *block* the change, but I'll push back against >>> the template syntax. >>> >>> >>> >>> Cheers, >>> >>> Jason >>> >>> >>> >>> On Fri, Sep 18, 2020 at 5:02 AM Gabe Black <gabebl...@google.com> wrote: >>> >>> I spent some more time digging into 2, and while I didn't find anything >>> that directly stated that you aren't allowed to do that, without explicit >>> support I think it flies in the face of how C++ templates, types, etc. work >>> to the point where if you *did* find a way to do it, it would almost >>> certainly be a bug or an oversight in the standard somewhere. So unless >>> they do adopt one of those standards I saw proposed and we wait a good >>> number of years for it to be implemented in all the compilers we support >>> (one said it targeted C++23) I think templates, while slightly gross, are >>> really the only way to make it work. >>> >>> >>> >>> Gabe >>> >>> >>> >>> On Thu, Sep 17, 2020 at 2:53 PM Jason Lowe-Power <ja...@lowepower.com> >>> wrote: >>> >>> >>> >>> >>> >>> On Thu, Sep 17, 2020 at 2:48 PM Gabe Black via gem5-dev < >>> gem5-dev@gem5.org> wrote: >>> >>> 1. Sounds good, I'll hopefully have some time to put together a CL in no >>> too long (weekend?). >>> >>> >>> >>> 2. I 5ries to figure out a way to do it without the template that wasn't >>> really gross a somewhat fragile and wasn't able to, but that would >>> definitely be preferable. I'll keep thinking about it, but the internet >>> didn't seem to have any ideas either. Unfortunately using constexpr won't >>> work like that Jason, although I wish it did and found a couple unadopted >>> (as far as I know) standards proposals to that effect. >>> >>> >>> >>> Yeah, that's what I found, too :). >>> >>> >>> >>> >>> >>> 3. Sounds good. Likely this weekend? >>> >>> >>> >>> Gabe >>> >>> >>> >>> On Thu, Sep 17, 2020, 1:15 PM Bobby Bruce via gem5-dev < >>> gem5-dev@gem5.org> wrote: >>> >>> 1) Seems fine to me. >>> >>> >>> >>> 2) I remember looking into this and I agree with Jason, it involves >>> template magic which I'm not a huge fan of. I feel like in order to add >>> these compile time asserts we'd be sacrificing some >>> readability/ease-of-usability of bitfields.hh. This may just be a "me >>> thing", but something about templates confuse me whenever I need to deal >>> with them. >>> >>> >>> >>> 3) In truth, our minimum supported Clang version is 3.9 in practise (We >>> even state on our website's building documentation that we support Clang >>> 3.9 to 9: http://www.gem5.org/documentation/general_docs/building >>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.gem5.org%2Fdocumentation%2Fgeneral_docs%2Fbuilding&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571740119&sdata=6hkyKwsWGm%2BY67faMlI2NvDQR21oA6h0L2fDRekCD7o%3D&reserved=0>). >>> I didn't realize we still have "3.1" hardcoded in the SConscript and would >>> be happy for this to be bumped up to 3.9. Our compiler tests do not test >>> with versions of clang before 3.9, so, at present, we aren't doing much to >>> help those using versions older than 3.9. I'd love to bump up to c++14 >>> also. While I'm sure there are plenty of good reasons, I personally would >>> like to use C++14's deprecation attribute for if/when we start deprecating >>> gem5 C++ APIs: >>> https://en.cppreference.com/w/cpp/language/attributes/deprecated >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.cppreference.com%2Fw%2Fcpp%2Flanguage%2Fattributes%2Fdeprecated&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571750113&sdata=dazAk2W0t1y04k%2BlD2Eu3acXTZ5YjrGHlLQNMM7PG10%3D&reserved=0> >>> >>> >>> >>> We already do use the deprecated attribute (see >>> https://gem5.googlesource.com/public/gem5/+/refs/heads/develop/src/base/compiler.hh#55 >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgem5.googlesource.com%2Fpublic%2Fgem5%2F%2B%2Frefs%2Fheads%2Fdevelop%2Fsrc%2Fbase%2Fcompiler.hh%2355&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571770101&sdata=ZeptqljKIK1C8oAbFd%2Frfz0eRmONiIk1kp4%2BomWoenk%3D&reserved=0> >>> ). >>> >>> >>> >>> We should be able to get rid of this: >>> https://gem5.googlesource.com/public/gem5/+/refs/heads/develop/src/base/compiler.hh#93 >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgem5.googlesource.com%2Fpublic%2Fgem5%2F%2B%2Frefs%2Fheads%2Fdevelop%2Fsrc%2Fbase%2Fcompiler.hh%2393&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571770101&sdata=ufibvsE%2FQ7NY7IupAGJATGRxV6NWGvUCdPbiWX47tX8%3D&reserved=0> >>> >>> And maybe this: >>> https://gem5.googlesource.com/public/gem5/+/refs/heads/develop/src/base/compiler.hh#69 >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgem5.googlesource.com%2Fpublic%2Fgem5%2F%2B%2Frefs%2Fheads%2Fdevelop%2Fsrc%2Fbase%2Fcompiler.hh%2369&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571780090&sdata=WoB63rD6sFKr81ufEMRQH0YEMLipELBGmApFrW9uoDw%3D&reserved=0> >>> >>> >>> >>> >>> >>> -- >>> >>> Dr. Bobby R. Bruce >>> Room 2235, >>> Kemper Hall, UC Davis >>> Davis, >>> CA, 95616 >>> >>> >>> >>> web: https://www.bobbybruce.net >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.bobbybruce.net%2F&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571780090&sdata=2B7Shdd7ko8cltS6slVYXSC84F3DCOkfcm04SaLxD80%3D&reserved=0> >>> >>> >>> >>> >>> >>> On Thu, Sep 17, 2020 at 8:25 AM Jason Lowe-Power via gem5-dev < >>> gem5-dev@gem5.org> wrote: >>> >>> Hey Gabe, >>> >>> >>> >>> On Thu, Sep 17, 2020 at 4:46 AM Gabe Black via gem5-dev < >>> gem5-dev@gem5.org> wrote: >>> >>> 1. Use __builtin_expect() for panic, fatal, etc. Preexisting library >>> functions like assert probably already have this, but our versions don't >>> and have similar behavior patterns. This should improve performance. >>> >>> >>> >>> Seems like a good idea. It shouldn't hurt anything. >>> >>> >>> >>> >>> >>> 2. Create template versions of the bits, etc functions in bitfields.hh >>> which use static_assert to verify that the bounds are in the right order. >>> Unless the bounds change at runtime (probably very uncommon in practice) >>> >>> >>> >>> I like the idea of static asserts, but don't like the change in the >>> syntax away from a simple function call. >>> >>> >>> >>> Would it be possible to instead use a constexpr function parameter? >>> >>> >>> >>> What we would really like is >>> >>> >>> >>> template <class T> >>> inline >>> T >>> bits(T val, *constexpr *int first, *constexpr *int last) >>> { >>> int nbits = first - last + 1; >>> *static*_assert((first - last) >= 0); >>> return (val >> last) & mask(nbits); >>> } >>> >>> >>> >>> However, after spending 15-30 minutes researching it doesn't seem like >>> this is easy to do today. Gabe... you seem to know much more template >>> magic. Maybe there's a way? >>> >>> >>> >>> >>> >>> bits(foo, 2, 1) => bits<2, 1>(foo) >>> >>> >>> >>> Then we get the free compile time checking of bounds most of the time >>> without big overhead otherwise. Something like this: >>> >>> >>> >>> template <int first, int last, typename T> >>> >>> constexpr T >>> >>> bits(T val) >>> >>> { >>> >>> static_assert(first > last); >>> >>> return bits(val, first, last); >>> >>> } >>> >>> >>> >>> 3. Our new min gcc is version 5 which supports c++14. Our min clang is >>> 3.1 which does not, but 3.4 does. Do we want to bump the min clang version >>> up and move from C++11 to C++14? C++17 has more compelling features, but >>> C++14 fixed some annoyances, at least according to wikipedia: >>> >>> >>> >>> https://en.wikipedia.org/wiki/C%2B%2B14 >>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FC%252B%252B14&data=02%7C01%7Canthony.gutierrez%40amd.com%7C8dda55bf862b42f3f16408d85be45700%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637360383571790094&sdata=q2adNzwC7u7FVbFATbG%2F%2BG3lokPQ%2BhjtcaJSHHuCnXg%3D&reserved=0> >>> >>> >>> >>> Yeah, I don't see any reason not to bump our minimum clang version. If >>> we do go up to c++14, we can simplify compiler.hh significantly. >>> >>> >>> >>> Thanks for starting this conversation! >>> >>> >>> >>> Cheers, >>> >>> Jason >>> >>> >>> >>> >>> >>> >>> >>> Gabe >>> >>> _______________________________________________ >>> gem5-dev mailing list -- gem5-dev@gem5.org >>> To unsubscribe send an email to gem5-dev-le...@gem5.org >>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >>> >>> _______________________________________________ >>> gem5-dev mailing list -- gem5-dev@gem5.org >>> To unsubscribe send an email to gem5-dev-le...@gem5.org >>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >>> >>> _______________________________________________ >>> gem5-dev mailing list -- gem5-dev@gem5.org >>> To unsubscribe send an email to gem5-dev-le...@gem5.org >>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >>> >>> _______________________________________________ >>> gem5-dev mailing list -- gem5-dev@gem5.org >>> To unsubscribe send an email to gem5-dev-le...@gem5.org >>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >>> >>>
_______________________________________________ gem5-dev mailing list -- gem5-dev@gem5.org To unsubscribe send an email to gem5-dev-le...@gem5.org %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
