I agree with all of this.
I'm not sure it's worth the change right now. A quick grep of "bits(" shows
that there's many cases (maybe 5-10%?) where there are variables used for
the bits. I suspect that most of these are constexpr, but it's impossible
to tell without checking each one. Just for ease of this change, I think
that we should keep the same function name in both cases.
Approximately all places bits() is called with only numbers as the 2nd and
3rd arguments.
> grep -E "bits\(.*, *[0-9]{1,2}, *[0-9]{1,2}\)" src -r | wc -l
2271
Approximately all places bits() is called with 3 arguments.
> grep -E "bits\(.*,.*,.*\)" src -r | wc -l
2589
Cheers,
Jason
On Fri, Sep 18, 2020 at 8:49 PM Gabe Black <[email protected]> wrote:
> 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 <[email protected]>
> 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 <[email protected]>
>> 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 <
>>> [email protected]> 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 <[email protected]>
>>>> *Sent:* Friday, September 18, 2020 8:05 AM
>>>> *To:* Gabe Black <[email protected]>
>>>> *Cc:* gem5 Developer List <[email protected]>; Jason Lowe-Power <
>>>> [email protected]>
>>>> *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 <[email protected]>
>>>> 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 <[email protected]>
>>>> wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Thu, Sep 17, 2020 at 2:48 PM Gabe Black via gem5-dev <
>>>> [email protected]> 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 <
>>>> [email protected]> 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 <
>>>> [email protected]> wrote:
>>>>
>>>> Hey Gabe,
>>>>
>>>>
>>>>
>>>> On Thu, Sep 17, 2020 at 4:46 AM Gabe Black via gem5-dev <
>>>> [email protected]> 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 -- [email protected]
>>>> To unsubscribe send an email to [email protected]
>>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>>>
>>>> _______________________________________________
>>>> gem5-dev mailing list -- [email protected]
>>>> To unsubscribe send an email to [email protected]
>>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>>>
>>>> _______________________________________________
>>>> gem5-dev mailing list -- [email protected]
>>>> To unsubscribe send an email to [email protected]
>>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>>>
>>>> _______________________________________________
>>>> gem5-dev mailing list -- [email protected]
>>>> To unsubscribe send an email to [email protected]
>>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>>>
>>>>
_______________________________________________
gem5-dev mailing list -- [email protected]
To unsubscribe send an email to [email protected]
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
