RFR: JDK-8331732 : [PPC64] Unify and optimize code which converts != 0 to 1 [v4]

Martin Doerr mdoerr at openjdk.org
Thu Jun 27 16:42:14 UTC 2024


On Thu, 27 Jun 2024 16:27:36 GMT, Suchismith Roy <sroy at openjdk.org> wrote:

>> [JDK-8331732](https://bugs.openjdk.org/browse/JDK-8331732)
>> The template interpreter contains branch-free conversion code for T_BOOLEAN (TemplateInterpreterGenerator::generate_result_handler_for).
>> 
>> SharedRuntime::generate_native_wrapper uses unoptimized code to "Unpack the native result" for T_BOOLEAN.
>> Power10 has the "setbc" / "setbcr" instruction.
>> 
>> A new function has been created for the conversion and use "setbcr" on Power10 (determined by VM_Version::has_brw()) and otherwise the branch-free implementation. We should have a function for 32 and one for 64 bit operations (or one with supports both).
>> 
>> The new code for MacroAssembler::verify_secondary_supers_table  also uses the new function.
>
> Suchismith Roy has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Comments and spaces
>  - Comments and spaces

Thanks! This looks good now, except very minor nits.

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 27:

> 25: 
> 26: #include "precompiled.hpp"
> 27: #include "asm/assembler.inline.hpp"

I think this is not needed because it is included via `macroAssembler.inline.hpp`.

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 351:

> 349: 
> 350:   if (VM_Version::has_brw()) {
> 351:     if(is_64bit) {

Coding style should use a whitespace: `if (`

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 354:

> 352:       cmpdi(CCR0, dst, 0);
> 353:     }
> 354:     else {

Coding style: `} else {`

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 359:

> 357:     setbcr(dst, CCR0, Assembler::zero);
> 358:   }
> 359:   else {

Coding style: `} else {`

src/hotspot/cpu/ppc/macroAssembler_ppc.cpp line 362:

> 360:     neg(temp, dst);
> 361:     orr(temp, dst, temp);
> 362:     if(is_64bit) {

Coding style should use a whitespace: `if (`

-------------

PR Review: https://git.openjdk.org/jdk/pull/19886#pullrequestreview-2145925527
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1657457757
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1657449820
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1657450572
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1657452613
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1657451092


More information about the hotspot-dev mailing list