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 fmin/fmax/dmin/dmax on Z #20716

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

matthewhall2
Copy link
Contributor

@matthewhall2 matthewhall2 commented Nov 29, 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

Uses SIMD instructions for floats on z14+, and for doubles on z13+ (fixes errors on z13 machines that were seen after #20530).

https://github.ibm.com/runtimes/openj9-jit-z/issues/935#issuecomment-98667018

depends on eclipse-omr/omr#7572

vmfarm
vmfarm w recognizedmethodtests

@matthewhall2
Copy link
Contributor Author

please review @r30shah

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpicks, changes looks ok to me. Please fix this and relaunch the build in internal farm again for sanity.

@@ -1,33 +1,42 @@
/*
* Copyright IBM Corp. and others 2019
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original copy right was correct, there was not issue with space here.

public class TestJavaLangMath {

/**
* Tests the constant corner cases defined by the {@link Math.sqrt} method.
* Tests the constant corner cases defined by the {@link Math.sqrt} method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be consistent for comments.

/**
 *
 *
 */

@@ -0,0 +1,292 @@
/*
* Copyright IBM Corp. and others 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the space here. Check the copyright in other files.

Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I will wait for OMR changes to propagate before launching tests.

@matthewhall2
Copy link
Contributor Author

Minor nitpicks, changes looks ok to me. Please fix this and relaunch the build in internal farm again for sanity.

Thanks @r30shah
vmfarm run

@r30shah
Copy link
Contributor

r30shah commented Dec 6, 2024

@matthewhall2 Can you resolve the merge conflict? I see that your OMR changes have been propagated to openj9-omr so we can target to merge this today

- Adds java_lang_Math_max/min for 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]>
- use new omr min/max helper
- +0.0 compares as strictly greater than -0.0
- returns first NaN unchanged if present

Signed-off-by: Matthew Hall <[email protected]>
- tests float corner cases with +/-0
- NaN cases
- confirms respective OMR changes

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

@matthewhall2 Can you resolve the merge conflict? I see that your OMR changes have been propagated to openj9-omr so we can target to merge this today

done. Running internal sanity checks again

@r30shah
Copy link
Contributor

r30shah commented Dec 6, 2024

jenkins test sanity zlinux jdk11,jdk21

@matthewhall2
Copy link
Contributor Author

matthewhall2 commented Dec 6, 2024

still waiting on the internal sanity run @r30shah. will let you know when it finishes (the only failures right now are for x86)

@r30shah
Copy link
Contributor

r30shah commented Dec 6, 2024

All the tests have been on jenkins passes. Merging this based on the the sanity run build from #20716 (comment) and #20716 (comment).

@matthewhall2 We should keep monitor OpenJ9 Jenkins build and internal builds in case something pop-up.

@r30shah r30shah merged commit 1a808c0 into eclipse-openj9:master Dec 6, 2024
9 checks passed
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.

2 participants