Re: RFR: JDK-8300783: Consolidate byteswap implementations [v8]
Deduplicate byte swapping implementations by consolidating them into `utilities/byteswap.hpp`, following `std::byteswap` introduced in C++23. Further simplification of `Bytes` will follow in https://github.com/openjdk/jdk/pull/12078.
Justin King has updated the pull request incrementally with one additional commit since the last revision: Update comment Signed-off-by: Justin King <jcking@google.com> ------------- Changes: - all: https://git.openjdk.org/jdk/pull/12114/files - new: https://git.openjdk.org/jdk/pull/12114/files/428520cc..f6996e1a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12114&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12114&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12114.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12114/head:pull/12114 PR: https://git.openjdk.org/jdk/pull/12114
On Tue, 24 Jan 2023 16:14:22 GMT, Justin King <jcking@openjdk.org> wrote:
Deduplicate byte swapping implementations by consolidating them into `utilities/byteswap.hpp`, following `std::byteswap` introduced in C++23. Further simplification of `Bytes` will follow in https://github.com/openjdk/jdk/pull/12078.
Justin King has updated the pull request incrementally with one additional commit since the last revision:
Update comment
Signed-off-by: Justin King <jcking@google.com>
Cleanup/consolidation looks really good. Have to wonder if we can go further with `get_Java_u2` etc, in a next step? The actual byteswap stuff seems okay but not really my area and compiler specific actions are not something I'm at all familiar with. I will grab this and run through our CI. Thanks. ------------- PR: https://git.openjdk.org/jdk/pull/12114
On Wed, 25 Jan 2023 01:30:25 GMT, David Holmes <dholmes@openjdk.org> wrote:
Have to wonder if we can go further with `get_Java_u2` etc, in a next step?
Probably. We can visit that in https://github.com/openjdk/jdk/pull/12078 after this one. We might be able to do something like `LittleEndian` and `BigEndian` where each has `store`, `store_unaligned`, `load`, and `load_unaligned`. `LittleEndian` would convert native to little endian for store and do the inverse for load. `BigEndian` would do the same for big endian. In cases where they are the same to native, its a noop. uint32_t* aligned_ptr = /* */; void* unaligned_ptr = /* */; BigEndian::store(aligned_ptr, 1); BigEndian::load(aligned_ptr); BigEndian::store_unaligned<uint32_t>(unaligned_ptr, 1); BigEndian::load_unaligned<uint32_t>(unaligned_ptr); Maybe. Just a thought. ------------- PR: https://git.openjdk.org/jdk/pull/12114
On Tue, 24 Jan 2023 16:14:22 GMT, Justin King <jcking@openjdk.org> wrote:
Deduplicate byte swapping implementations by consolidating them into `utilities/byteswap.hpp`, following `std::byteswap` introduced in C++23. Further simplification of `Bytes` will follow in https://github.com/openjdk/jdk/pull/12078.
Justin King has updated the pull request incrementally with one additional commit since the last revision:
Update comment
Signed-off-by: Justin King <jcking@google.com>
CI run was fine wrt these changes. ------------- PR: https://git.openjdk.org/jdk/pull/12114
On Wed, 25 Jan 2023 12:31:11 GMT, David Holmes <dholmes@openjdk.org> wrote:
Justin King has updated the pull request incrementally with one additional commit since the last revision:
Update comment
Signed-off-by: Justin King <jcking@google.com>
CI run was fine wrt these changes.
@dholmes-ora I found more duplicate byteswap implementations. One in `src/hotspot/cpu/ppc/stubRoutines_ppc_64.cpp` and another in `utilities/moveBits.hpp`. `utilities/moveBits.hpp` is now just bit reversal, as none of the other functions are called anywhere. It should probably be renamed... I also upgraded `utilities/moveBits.hpp` to use `__builtin_bitreverse` from Clang when available. I am not aware of MSVC or XLC having a bit reversal intrinsic. ------------- PR: https://git.openjdk.org/jdk/pull/12114
On Wed, 25 Jan 2023 12:31:11 GMT, David Holmes <dholmes@openjdk.org> wrote:
Justin King has updated the pull request incrementally with one additional commit since the last revision:
Update comment
Signed-off-by: Justin King <jcking@google.com>
CI run was fine wrt these changes.
@dholmes-ora Poke. :) ------------- PR: https://git.openjdk.org/jdk/pull/12114
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. ---------------------------------------------------------------------- On Mon, 6 Mar 2023 16:27:26 GMT, Justin King <jcking@openjdk.org> wrote:
CI run was fine wrt these changes.
@dholmes-ora Poke. :)
Sorry @jcking but as I previously indicated I'm not qualified to review the details of this. But I will see if I can get someone else to. ------------- PR: https://git.openjdk.org/jdk/pull/12114
On Mon, 6 Mar 2023 16:27:26 GMT, Justin King <jcking@openjdk.org> wrote:
CI run was fine wrt these changes.
@dholmes-ora Poke. :)
@jcking, as Committer, you can integrate it now. ------------- PR: https://git.openjdk.org/jdk/pull/12114
participants (3)
-
David Holmes
-
Justin King
-
Vladimir Kozlov