8349632: RISC-V: Add Zfa fminm/fmaxm #23509

Anjian-Wen · 2025-02-07T06:52:13Z

Add RISCV zfa extension fminm/fmaxm
This two new Floating-point instructions can deal with NaN input directly, which can decrease instructions when calculate min or max

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8349632: RISC-V: Add Zfa fminm/fmaxm (Enhancement - P4)

Reviewers

Fei Yang (@RealFYang - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23509/head:pull/23509
$ git checkout pull/23509

Update a local copy of the PR:
$ git checkout pull/23509
$ git pull https://git.openjdk.org/jdk.git pull/23509/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23509

View PR using the GUI difftool:
$ git pr show -t 23509

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23509.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-07T06:52:42Z

Hi @Anjian-Wen, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Anjian-Wen" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-02-07T06:52:56Z

@Anjian-Wen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8349632: RISC-V: Add Zfa fminm/fmaxm

Reviewed-by: fyang

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RealFYang) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-02-07T06:53:23Z

@Anjian-Wen The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Anjian-Wen · 2025-02-07T07:01:38Z

/covered

bridgekeeper · 2025-02-07T07:02:29Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

openjdk · 2025-02-07T11:45:48Z

@Anjian-Wen Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

Anjian-Wen · 2025-02-18T03:27:06Z

/covered show my email address in company, and recommit for review

Hi @Anjian-Wen, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Anjian-Wen" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

bridgekeeper · 2025-02-18T03:27:37Z

Already processed the covered command

RealFYang · 2025-02-21T04:03:11Z

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp


+void C2_MacroAssembler::minmmaxm_fp(FloatRegister dst, FloatRegister src1, FloatRegister src2,
+                                  bool is_double, bool is_min) {
+  assert_different_registers(dst, src1, src2);


Do FMINM.S and FMAXM.S have a constraint on the registers?

Thanks for replying.
From Zfa doc https://github.com/riscv/riscv-isa-manual/blob/main/src/zfa.adoc, FMINM.S/FMAXM.S look defined like the FMIN.S and FMAX.S instructions. And from the ISA manual, I have not found any constraints on the registers. it only needs normal floating point registers

Anjian-Wen · 2025-02-21T07:46:22Z

/open

openjdk · 2025-02-21T07:47:09Z

@Anjian-Wen This pull request is now open

delete assert and change fminm/fmaxm to new match rule

Anjian-Wen · 2025-02-21T08:44:19Z

/issue JDK-8349632

openjdk · 2025-02-21T08:44:39Z

@Anjian-Wen The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

delete assert in new add macroAssembly but not the old

add zfa predicate

RealFYang · 2025-02-22T01:28:56Z

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp

  bind(Done);
 }

+void C2_MacroAssembler::minmmaxm_fp(FloatRegister dst, FloatRegister src1, FloatRegister src2,


There is no need to have this macro-assembler routine. You can call fminm_s/d / fmaxm_s/d directly on the callsites.

Change macro-assembler routine to directly call in riscv.ad

Anjian-Wen · 2025-03-03T03:38:30Z

Hi @Anjian-Wen, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user Anjian-Wen" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

/signed

bridgekeeper · 2025-03-03T03:39:16Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

mlbridge · 2025-03-11T19:23:09Z

Webrevs

RealFYang · 2025-03-12T02:27:27Z

@Anjian-Wen : Do you have github actions enabled? Usually there are about 16 checks, but you only have 2 check passed

Anjian-Wen · 2025-03-12T02:39:02Z

@Anjian-Wen : Do you have github actions enabled? Usually there are about 16 checks, but you only have 2 check passed

I have choose “Allow all actions and reusable workflows” in my repository setting but it still remains 1 test，is there any other option I should choose？

Anjian-Wen · 2025-03-12T07:45:05Z

This patch passed gtests, and also passed Related test -- test/jdk/java/lang/Math/MinMax.java in qemu with -XX:+UseZfa

RealFYang

Looks good. hotspot:tier1 test good using qemu-system with UseZfa enabled by default.
BTW: Similar thing could be done for the float16 variant after #23844 is merged.

openjdk · 2025-03-13T00:34:59Z

⚠️ @Anjian-Wen the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout Zfa_dev_branch
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

Anjian-Wen · 2025-03-14T05:47:17Z

/integrate

openjdk · 2025-03-14T05:47:58Z

@Anjian-Wen
Your change (at version c65798a) is now ready to be sponsored by a Committer.

RealFYang · 2025-03-14T05:52:20Z

/sponsor

openjdk · 2025-03-14T05:53:06Z

Going to push as commit a7a09f6.

openjdk · 2025-03-14T05:53:13Z

@RealFYang @Anjian-Wen Pushed as commit a7a09f6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

bridgekeeper bot added the oca Needs verification of OCA signatory status label Feb 7, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 7, 2025

Anjian-Wen changed the title ~~8349632: RISC-V: Add Zfa Minimum and Maximum Instructions~~ 8349632: RISC-V: Add Zfa fminm/fmaxm Feb 7, 2025

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Feb 7, 2025

8349632:RISC-V: Add Zfa fminm/fmaxm

02e1562

Anjian-Wen force-pushed the Zfa_dev_branch branch from 4e06f81 to 02e1562 Compare February 7, 2025 11:45

RealFYang reviewed Feb 21, 2025

View reviewed changes

Anjian-Wen changed the title ~~8349632: RISC-V: Add Zfa fminm/fmaxm~~ RISC-V: Add Zfa fminm/fmaxm Feb 21, 2025

Anjian-Wen closed this Feb 21, 2025

Anjian-Wen deleted the Zfa_dev_branch branch February 21, 2025 07:35

Anjian-Wen restored the Zfa_dev_branch branch February 21, 2025 07:37

openjdk bot reopened this Feb 21, 2025

JDK-8349632: RISCV: Add Zfa fminm/fmaxm

1d22e6f

delete assert and change fminm/fmaxm to new match rule

Anjian-Wen changed the title ~~RISC-V: Add Zfa fminm/fmaxm~~ 8349632: RISC-V: Add Zfa fminm/fmaxm Feb 21, 2025

Anjian-Wen changed the title ~~8349632: RISC-V: Add Zfa fminm/fmaxm~~ RISC-V: Add Zfa fminm/fmaxm Feb 21, 2025

openjdk bot changed the title ~~RISC-V: Add Zfa fminm/fmaxm~~ 8349632: RISC-V: Add Zfa fminm/fmaxm Feb 21, 2025

Anjian-Wen added 2 commits February 21, 2025 16:51

8349632: RISC-V: Add Zfa fminm/fmaxm

defa74d

delete assert in new add macroAssembly but not the old

JDK-8349632: RISC-V: Add Zfa fminm/fmaxm

037a364

add zfa predicate

RealFYang reviewed Feb 22, 2025

View reviewed changes

8349632: RISC-V: Add Zfa fminm/fmaxm

b763ee4

Change macro-assembler routine to directly call in riscv.ad

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Mar 11, 2025

openjdk bot added the rfr Pull request is ready for review label Mar 11, 2025

Merge branch 'openjdk:master' into Zfa_dev_branch

927d024

Anjian-Wen added 2 commits March 12, 2025 11:34

add temp commit for test

faa9070

delete useless comment

249bcd4

RealFYang approved these changes Mar 13, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2025

Merge branch 'openjdk:master' into Zfa_dev_branch

c65798a

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 14, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 14, 2025

openjdk bot closed this Mar 14, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 14, 2025

Anjian-Wen deleted the Zfa_dev_branch branch April 27, 2025 06:11

8349632: RISC-V: Add Zfa fminm/fmaxm #23509

8349632: RISC-V: Add Zfa fminm/fmaxm #23509

Uh oh!

Conversation

Anjian-Wen commented Feb 7, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 7, 2025

Uh oh!

openjdk bot commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 7, 2025

Uh oh!

Anjian-Wen commented Feb 7, 2025

Uh oh!

bridgekeeper bot commented Feb 7, 2025

Uh oh!

openjdk bot commented Feb 7, 2025

Uh oh!

Anjian-Wen commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bridgekeeper bot commented Feb 18, 2025

Uh oh!

RealFYang Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

Anjian-Wen Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

Anjian-Wen commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

Anjian-Wen commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

RealFYang Feb 22, 2025

Choose a reason for hiding this comment

Uh oh!

Anjian-Wen commented Mar 3, 2025

Uh oh!

bridgekeeper bot commented Mar 3, 2025

Uh oh!

mlbridge bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

RealFYang commented Mar 12, 2025

Uh oh!

Anjian-Wen commented Mar 12, 2025

Uh oh!

Anjian-Wen commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RealFYang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Mar 13, 2025

Uh oh!

Anjian-Wen commented Mar 14, 2025

Uh oh!

openjdk bot commented Mar 14, 2025

Uh oh!

RealFYang commented Mar 14, 2025

Uh oh!

openjdk bot commented Mar 14, 2025

Uh oh!

openjdk bot commented Mar 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Anjian-Wen commented Feb 7, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 7, 2025 •

edited

Loading

Anjian-Wen commented Feb 18, 2025 •

edited

Loading

mlbridge bot commented Mar 11, 2025 •

edited

Loading

Anjian-Wen commented Mar 12, 2025 •

edited

Loading

RealFYang left a comment •

edited

Loading