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

[Martin Doerr]

On Wed, 26 Jun 2024 16:52:35 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 one additional commit since the last revision:
> 
>   space after comma

Changes requested by mdoerr (Reviewer).

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1785:

> 1783:   inline void setnbc(Register d, ConditionRegister cr, Condition cc);
> 1784:   inline void setbcr( Register d, int biint);
> 1785:   inline void setbcr( Register d, ConditionRegister cr, Condition cc);

Extra whitespace. Please remove.

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

> 344:   return (long) x;
> 345: }
> 346: // Branch-free implementation to convert !0 to false

Please change to "!=0 to 1"

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

> 346: // Branch-free implementation to convert !0 to false
> 347: // Set register dst to true if dst is non zero using temp for calculations on Power Version<10.
> 348: // Set register dst to true if dst is non zero for Power 10 and above machines.

Please don't use the term "true". It's not defined in assembler.
In addition: We do the same for older processors. Just by other instructions.
I think these 2 lines could be removed.

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

> 350: 
> 351:   if (VM_Version::has_brw()) {
> 352:     if(use_64bit)

Please use hotspot coding style: `if (use_64bit) {`
Curly braces should also be used for single statements.

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

> 2406: 
> 2407:   // convert !=0 to 1
> 2408:   normalize_bool(result, R0, true);

There's a second usage in this function. Please also replace that one.

src/hotspot/cpu/ppc/macroAssembler_ppc.hpp line 297:

> 295:   };
> 296: 
> 297:   // Branch-free implementation to convert !0 to false.

"!=0"

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

PR Review: https://git.openjdk.org/jdk/pull/19886#pullrequestreview-2144614270
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1656680482
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1656684654
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1656688689
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1656699697
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1656705233
PR Review Comment: https://git.openjdk.org/jdk/pull/19886#discussion_r1656706013