<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body><div style="font-family: sans-serif;"><div class="markdown" style="white-space: normal;">
<p dir="auto">After sleeping on it I realize I like Kim’s improvements<br>
better than my code which does not derive the type <code style="margin: 0; padding: 0 0.4em; border-radius: 3px; background-color: #F7F7F7;">I</code>.</p>
<p dir="auto">The use of metaprogramming (or whatever it is) to derive<br>
the ad hoc type <code style="margin: 0; padding: 0 0.4em; border-radius: 3px; background-color: #F7F7F7;">I</code> using funny conditionals does in fact<br>
make for logic that more clearly expresses how the integral<br>
promotion rules of C++ will affect the outcome.</p>
<p dir="auto">Suggestion: To the gtest unit test, add the following block<br>
to advise folks how to check code quality:</p>
<pre style="margin-left: 15px; margin-right: 15px; padding: 5px; background-color: #F7F7F7; border-radius: 5px 5px 5px 5px; overflow-x: auto; max-width: 90vw;"><code style="margin: 0; border-radius: 3px; background-color: #F7F7F7; padding: 0px;">// To manually check code quality, try these commands:
//
// $ make hotspot exploded-test TEST=gtest:moveBits
// $ objdump -D $(find build -name test_moveBits.o) > /tmp/foo
// $ sed < /tmp/foo -n /quality/,/stream/p | grep -B20 bswap
//
// Or adapt $(find build -name test_moveBits.o.cmdline)
// replacing -c by -S and -o test_moveBits.s.
</code></pre>
<p dir="auto">On 26 Aug 2022, at 1:07, John Rose wrote:</p>
</div><blockquote class="embedded" style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; color: #777777;"><div id="F97ABBDA-A3DB-4D82-BA03-9A6E313E6525">
<div style="font-family: sans-serif;">
<div class="markdown" style="white-space: normal;">
<p dir="auto">Thanks; right. I like your improvements, except the metaprogramming.</p>
<p dir="auto">Here’s what I got:</p>
<p dir="auto"><a href="https://github.com/openjdk/jdk/compare/master...rose00:jdk:moveBits" style="color: #3983C4;">https://github.com/openjdk/jdk/compare/master...rose00:jdk:moveBits</a></p>
<p dir="auto">I’m not against metaprogramming when we need it, but there are alternatives here to deriving the extra types; the tricks are explained in the comments. I suppose it’s a matter of taste whether the metaprogramming is more or less obscure than the bit-twiddling tricks, but I will observe that bit-twiddling tricks are the focus of this file.</p>
<p dir="auto">I verified by hand that you get good code in all cases, even without explicit mentions of the intrinsics. Here’s an easy way to start that process:</p>
<pre style="margin-left: 15px; margin-right: 15px; padding: 5px; background-color: #F7F7F7; border-radius: 5px 5px 5px 5px; overflow-x: auto; max-width: 90vw;"><code style="margin: 0; border-radius: 3px; background-color: #F7F7F7; padding: 0px;">$ make hotspot exploded-test TEST=gtest:moveBits
$ objdump -D $(find build -name test_moveBits.o) > /tmp/foo
$ sed < /tmp/foo -n /quality/,/stream/p | grep -B20 bswap
</code></pre>
<p dir="auto">On 25 Aug 2022, at 23:39, Kim Barrett wrote:</p>
</div>
<div class="plaintext" style="white-space: normal;">
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; color: #777777;">
<p dir="auto">On Fri, 26 Aug 2022 01:06:25 GMT, John R Rose <jrose@openjdk.org> wrote:</p>
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; border-left-color: #999999; color: #999999;">
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; border-left-color: #BBBBBB; color: #BBBBBB;">
<p dir="auto">src/hotspot/share/utilities/moveBits.hpp line 31:</p>
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; border-left-color: #BBBBBB; color: #BBBBBB;">
<p dir="auto">29: #include "utilities/globalDefinitions.hpp"<br>
30:<br>
31: inline uint32_t reverse_bits_in_bytes_int(uint32_t x) {</p>
</blockquote>
<p dir="auto">Instead of two functions with different names (suffixed `_int` and `_long`), have one function name that has two overloads disambiguated by the size of the argument, e.g.</p>
<p dir="auto">template<typename T, ENABLE_IF(sizeof(T) <= 4)><br>
constexpr T reverse_bits_in_bytes(T x) { ... }</p>
<p dir="auto">template<typename T, ENABLE_IF(sizeof(T) == 8)><br>
constexpr T reverse_bits_in_bytes(T x) { ... }</p>
<p dir="auto">This allows `reverse_bits` to look something like</p>
<p dir="auto">template<typename T, ENABLE_IF(std::is_integral<T>::value)><br>
constexpr T reverse_bits(T v) {<br>
return reverse_bytes(reverse_bits_in_bytes(v));<br>
}</p>
</blockquote>
<p dir="auto">Yes, that could work.</p>
<p dir="auto">This also works, and feels appropriately short + sweet to me:</p>
<p dir="auto"><a href="https://github.com/rose00/jdk/pull/new/moveBits" style="color: #999999;">https://github.com/rose00/jdk/pull/new/moveBits</a></p>
<p dir="auto">(I also added a 90+% level gtest.)</p>
<p dir="auto">(Sorry; amended with `constexpr` and force-pushed…)</p>
</blockquote>
<p dir="auto">I took John's code and made various (I think) improvements:<br>
<a href="https://github.com/openjdk/jdk/compare/master...kimbarrett:openjdk-jdk:reverse-bits?expand=1" style="color: #777777;">https://github.com/openjdk/jdk/compare/master...kimbarrett:openjdk-jdk:reverse-bits?expand=1</a></p>
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; border-left-color: #999999; color: #999999;">
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; border-left-color: #BBBBBB; color: #BBBBBB;">
<p dir="auto">src/hotspot/share/utilities/moveBits.hpp line 63:</p>
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; border-left-color: #BBBBBB; color: #BBBBBB;">
<p dir="auto">61: /*****************************************************************************<br>
62: * GCC and compatible (including Clang)<br>
63: *****************************************************************************/</p>
</blockquote>
<p dir="auto">Is it really worth the extra code to allow the use of the gcc builtins?</p>
</blockquote>
<p dir="auto">Kim, is there a way to do this in less code?</p>
<p dir="auto">Seems to me that something like this could work:</p>
<p dir="auto">template<typename T, size_t S><br>
T reverse_bytes(T x) {<br>
if (size == 1) return x;<br>
T alt8bs = (T)((uint64_t)-1 / 0xFFFF * 0xFF); // 0x00FF…00FF<br>
x = ((x & alt8bs) << 8) | (((uint64_t)x >> 8 & alt8bs);<br>
if (size == 2) return x;<br>
T alt16bs = (T)((uint64_t)-1 / 0xFFFFFFFFll * 0xFFFF); // 0x0000FFFF…0000FFFF<br>
…<br>
}</p>
<p dir="auto">That is, one function bodies handles cases for 1/2/4/8 byte values.</p>
<p dir="auto">Could it be just a function template, with an explicit instantiation (overriding the general template) where it matches an intrinsic?</p>
</blockquote>
<p dir="auto">Answering my own question, no, it's not worth the extra code. gcc (and likely other compilers) are quite good at recognizing byte swapping code and transforming it into the appropriate platform-specific code like x86 bswap.</p>
<p dir="auto">-------------</p>
<br></blockquote>
</div>
<div class="markdown" style="white-space: normal;">
<blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #777777; color: #777777;">
<p dir="auto">PR: <a href="https://git.openjdk.org/jdk/pull/9987" style="color: #777777;">https://git.openjdk.org/jdk/pull/9987</a></p>
</blockquote>
</div>
</div></div></blockquote>
<div class="markdown" style="white-space: normal;">
</div></div></body>
</html>