RFR: 8375536: PPC64: Implement special MachNodes for floating point CMove [v2]

Richard Reingruber rrich at openjdk.org
Mon Jan 26 17:41:26 UTC 2026


On Wed, 21 Jan 2026 13:11:52 GMT, David Briemann <dbriemann at openjdk.org> wrote:

>> Adds the following mach nodes:
>> match(Set dst (CMoveF (Binary cop (CmpF op1 op2)) (Binary src1 src2)));
>> match(Set dst (CMoveD (Binary cop (CmpD op1 op2)) (Binary src1 src2)));
>> match(Set dst (CMoveF (Binary cop (CmpD op1 op2)) (Binary src1 src2)));
>> match(Set dst (CMoveD (Binary cop (CmpF op1 op2)) (Binary src1 src2)));
>
> David Briemann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   address review comments

Hi David,

nice work! ...with just a few rough edges ;)

I haven't yet finished the review but I still wanted to send you the comments I collected so far.

Cheers, Richard.

src/hotspot/cpu/ppc/c2_MacroAssembler_ppc.cpp line 669:

> 667: // Works for single and double precision floats.
> 668: // dst = (op1 cmp(cc) op2) ? src1 : src2;
> 669: void C2_MacroAssembler::cmovF(int cc, VectorSRegister dst, VectorSRegister op1, VectorSRegister op2,

I've had a pretty hard time understanding the usage of `cc`. I found that its value comes from `operand cmpOp` in the ppc.ad file. The `cmpOp` values are meant to be used for encoding `BO` and `BI` fields of instructions that have them (with `bc` aka Branch Conditional as prominent example).
The encoding is rather difficult to understand. Luckily the instructions used here don't have `BO` or `BI` fields. I'd suggest to use `BoolTest::mask` directly and map these to the appropriate instructions (swapping operands if necessary). I think you get the `BoolTest::mask` replacing `$cop$$cmpcode` with `$cop$$constant` (see also `to_assembler_cond` on aarch64).
I'd expect this to make the implementation a lot easier to understand.
(I'm too embarrassed to tell how long it took me to understand this version ;))

src/hotspot/cpu/ppc/c2_MacroAssembler_ppc.cpp line 674:

> 672:   VectorSRegister second = src2;
> 673:   int exchange = (~cc) & 8;
> 674:   if (exchange) {

hotspot-style.md suggestes "Avoid implicit conversions to bool".

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

PR Review: https://git.openjdk.org/jdk/pull/29281#pullrequestreview-3698655184
PR Review Comment: https://git.openjdk.org/jdk/pull/29281#discussion_r2728567004
PR Review Comment: https://git.openjdk.org/jdk/pull/29281#discussion_r2722123267


More information about the hotspot-compiler-dev mailing list