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
