Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Java Behaviour w.r.t Math.max and Math.min for Floating Points #20530

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

matthewhall2
Copy link
Contributor

@matthewhall2 matthewhall2 commented Nov 7, 2024

  • Enables inlining of Math.max/Math.min functions for floats and doubles
  • Implements Java standard
    • +0 compares as strictly greater than -0
    • if the first arg is a NaN, returns the NaN unchanged. If only the second arg is a NaN, returns that NaN unchanged

vmfarm: http://vmfarm.rtp.raleigh.ibm.com/build_info.php?build_id=81412

personal build test: https://hyc-runtimes-jenkins.swg-devops.com/job/Pipeline-Build-Test-Personal/24986/

- Adds java_lang_Math_max/min_float/double as a recognized method
- Adds a SupportsInlineMath_MaxMin_FD flag to the Z code generator
- Flag is only set in Z if the TR_disableInlineMath_MaxMin_FD
  environment variable is not set
- If the flag is set, call nodes are transformed to a functionally
  equivalent tree that uses fmin/fmax/dmin/dmax nodes

Signed-off-by: Matthew Hall <[email protected]>
- spearate evaluators for J9 vs OMR to support differing behaviour (OMR
  complies with IEEE_754, while J9 returns the first NaN (if present)
- +0.0 compares as strictly greater than -0.0

Signed-off-by: Matthew Hall <[email protected]>
Add functional tests for `Math.max` and `Math.min`:
- to test specific float and double corner cases
- test with NaN, +0, & -0, values to confirm respective omr changes
- test all possible execution paths

Signed-off-by: Matthew Hall <[email protected]>
@r30shah
Copy link
Contributor

r30shah commented Nov 11, 2024

@matthewhall2 Can this be move out of draft if your internal testing is finished and all good?

@matthewhall2
Copy link
Contributor Author

@matthewhall2 Can this be move out of draft if your internal testing is finished and all good?

A few jobs are still waiting to run, will move it out of draft once they finish

@matthewhall2 matthewhall2 marked this pull request as ready for review November 12, 2024 14:43
@r30shah
Copy link
Contributor

r30shah commented Nov 13, 2024

jenkins test sanity zlinux,xlinux jdk11,jdk21

@r30shah
Copy link
Contributor

r30shah commented Nov 14, 2024

Based on the build links shared and and passing JDK11/JDK21 sanity builds on the PR, and as explicit review was done where this PR, fixes the issue with the test, approving the change and merging this one.

@matthewhall2 Please keep an eye on the OpenJ9 Jenkins build and VMFarm builds if there is anything pops up.

@r30shah r30shah merged commit 261100b into eclipse-openj9:master Nov 14, 2024
15 checks passed
@r30shah r30shah self-assigned this Nov 14, 2024
@pshipton
Copy link
Member

Reverting due to #20594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants