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
