Support IEEE-754 for fmin/fmax/dmin/dmax nodes #7464

matthewhall2 · 2024-09-18T16:52:30Z

Enables inlining of fmin/fmax/dmin/dmax nodes
Implements IEEE-754 standard for the evaluators:
- if the first arg is a NaN, returns the corresponding quiet NaN, same for if only the second arg is a NaN
Refactors xmaxxminhelper to infer correct compare/branch/move instructions based on datatype; helper does not change NaNs, so can be used in other min/max evaluators which can extend the behaviour on NaNs as they see fit
Uses vector operations when available, defaults to branching if not
Supports inlining for Support Java Behaviour w.r.t Math.max and Math.min for Floating Points eclipse-openj9/openj9#20185

Depends on OMR Simplifier change #7471 (needed for eclipse-openj9/openj9#20185 to pass)

https://hyc-runtimes-jenkins.swg-devops.com/job/jvm.29.personal/34283/

matthewhall2 · 2024-10-04T20:44:03Z

can you review please @r30shah ?

r30shah · 2024-10-04T21:54:48Z

compiler/z/codegen/ControlFlowEvaluator.cpp


-   TR::Register* lhsReg = cg->gprClobberEvaluate(lhsNode);
-   TR::Register* rhsReg = cg->evaluate(rhsNode);
+   TR::Node * lhsNode = node->getChild(0);


Can we rename lhsNode and rhsNode ?

r30shah · 2024-10-07T20:55:03Z

compiler/z/codegen/ControlFlowEvaluator.cpp

      {
+         compareRROp = node->getOpCode().isDouble() ? TR::InstOpCode::CDBR : TR::InstOpCode::CEBR;


Seems like you have extra indentation here in the next else block

r30shah · 2024-10-07T20:56:27Z

compiler/z/codegen/ControlFlowEvaluator.cpp

+      }
+
+   TR::LabelSymbol* cFlowRegionEnd = generateLabelSymbol(cg);
+   TR::LabelSymbol* swap = generateLabelSymbol(cg);


I may have missed this in initial review, but swapValue or something meaningful name should be used.

r30shah · 2024-10-07T20:59:11Z

compiler/z/codegen/ControlFlowEvaluator.cpp

+   if (isFloatingPointOp)
+      {
+      /*
+      Check for NaN operands for float and double


Can you use doxygen styled block comment (like done in [1] for example)

[1]. https://github.com/eclipse/omr/blob/13d47ea4def377422793a23813b6b25c76bbd56d/compiler/z/codegen/OMRTreeEvaluator.cpp#L4917-L4930

r30shah · 2024-10-07T21:01:32Z

compiler/z/codegen/OMRTreeEvaluator.hpp

@@ -253,6 +253,8 @@ class OMR_EXTENSIBLE TreeEvaluator: public OMR::TreeEvaluator
   static TR::Register *MethodEnterHookEvaluator(TR::Node *node, TR::CodeGenerator *cg);
   static TR::Register *MethodExitHookEvaluator(TR::Node *node, TR::CodeGenerator *cg);
   static TR::Register *PassThroughEvaluator(TR::Node *node, TR::CodeGenerator *cg);
+   static TR::Register *fpMinMaxVectorHelper(TR::Node *node, TR::CodeGenerator *cg);


Either here or where the function is implemented, we should add a doxygen styled comments for function description(Take a look at functions in z/OMRTreeEvaluator.hpp [1])

[1]. https://github.com/eclipse/omr/blob/13d47ea4def377422793a23813b6b25c76bbd56d/compiler/z/codegen/OMRTreeEvaluator.hpp#L924-L951

r30shah · 2024-10-07T21:03:46Z

compiler/z/codegen/ControlFlowEvaluator.cpp

+      {
+      result = OMR::Z::TreeEvaluator::xmaxxminhelper(node, cg);
+      }
+   generateRREInstruction(cg, TR::InstOpCode::LTDBR, node, result, result);


We should add a brief explanation for these Load Test instructions.

r30shah · 2024-10-07T21:34:13Z

compiler/z/codegen/ControlFlowEvaluator.cpp

-static TR::Register*
-xmaxxminHelper(TR::Node* node, TR::CodeGenerator* cg, TR::InstOpCode::Mnemonic compareRROp, TR::InstOpCode::S390BranchCondition branchCond, TR::InstOpCode::Mnemonic moveRROp)
+TR::Register *
+OMR::Z::TreeEvaluator::xmaxxminhelper(TR::Node * node, TR::CodeGenerator * cg)


I think the original name xmaxminHelper should be ok (You missed the caps in Helper when refactoring).

r30shah · 2024-10-08T14:22:00Z

compiler/z/codegen/ControlFlowEvaluator.cpp

+      /* Check for NaN operands for float and double
+       * Support float and double +0/-0 comparisons adhering to IEEE 754 standard
+       * Checking if operands are equal, then branching to equalRegion, otherwise
+       * fall through for NaN case handling
+       */


Suggested change

/* Check for NaN operands for float and double

* Support float and double +0/-0 comparisons adhering to IEEE 754 standard

* Checking if operands are equal, then branching to equalRegion, otherwise

* fall through for NaN case handling

*/

/**

* Check for NaN operands for float and double

* Support float and double +0/-0 comparisons adhering to IEEE 754 standard

* Checking if operands are equal, then branching to equalRegion, otherwise

* fall through for NaN case handling

*/

r30shah · 2024-10-08T14:27:28Z

compiler/z/codegen/ControlFlowEvaluator.cpp

+TR::Register *
+OMR::Z::TreeEvaluator::fpMinMaxVectorHelper(TR::Node *node, TR::CodeGenerator *cg)
+   {
+   TR_ASSERT(node->getNumChildren() >= 1  || node->getNumChildren() <= 2, "node has incorrect number of children");


We should combine this two asserts.

r30shah

Last nitpicks

r30shah · 2024-10-08T14:29:24Z

compiler/z/codegen/OMRTreeEvaluator.hpp

@@ -254,6 +254,41 @@ class OMR_EXTENSIBLE TreeEvaluator: public OMR::TreeEvaluator
   static TR::Register *MethodExitHookEvaluator(TR::Node *node, TR::CodeGenerator *cg);
   static TR::Register *PassThroughEvaluator(TR::Node *node, TR::CodeGenerator *cg);

+   /** \brief
+    *    This is a helper function used for floating point max/min operations
+    *    when SIMD instructions are available. +0.0 compares as strictly


I think we should mention that it generates Vector Instructions when available for max/min operations.

r30shah · 2024-10-08T18:44:19Z

This PR has to be merged first. The only potential issue is that OpenJ9 will follow the IEEE spec and not the Java spec until the openj9 PR is merged

Looking at your openJ9 change, is this statement true? Without OpenJ9 change , Java should not be inlining max and min for floating point types right >?

matthewhall2 · 2024-10-08T19:12:27Z

Looking at your openJ9 change, is this statement true? Without OpenJ9 change , Java should not be inlining max and min for floating point types right >?

ah yes, they won't be inlined without the openj9 change. But the openj9 PR won't compile without this PR, so this one still needs to be merged first.

matthewhall2 · 2024-10-09T18:49:42Z

@hzongaro can you review when you get the chance?

Add Math.max/min/F/D to list of recognized Java methods to: - Disable special aliasing rules since they are intrinsically optimized - Disable implicit asynchronous checks Signed-off-by: Sarwat Shaheen <[email protected]>

hzongaro

I think the changes look good overall. Just a few minor comments.

May I also ask you to update the commit comments to indicate that these changes are for inline code for z/Architecture only?

compiler/z/codegen/ControlFlowEvaluator.cpp

hzongaro

Looks good. Just one further minor comment.

compiler/z/codegen/ControlFlowEvaluator.cpp

- changes are only for the floating point min/max evaluators on z/Architecture - +0.0 compares as strictly greater than -0.0 - Refactored xmaxxminHelper to into a more generic helper function that does not change NaNs - added helper for fmin/fmax/... nodes that uses SIMD instructions when available that has same behaviour as xmaxxminHelper on floats - idea is to enable other evaluators to use these helpers and deal with NaNs as they see fit - omr evaluators support IEEE-754 w.r.t zeros and NaNs (returns the quiet NaN corresponding to the first NaN given if present) Signed-off-by: Matthew Hall <[email protected]>

r30shah

Thanks @matthewhall2 for the changes, it looks good to me, will wait for @hzongaro to approve and merge this before I go on and look at the J9 changes.

r30shah · 2024-10-18T19:04:04Z

jenkins build zos,zlinux

hzongaro

Looks good. Thanks for addressing the comments!

github-actions bot added arch:z comp:compiler labels Sep 18, 2024

matthewhall2 mentioned this pull request Sep 18, 2024

Support Java Behaviour w.r.t Math.max and Math.min for Floating Points eclipse-openj9/openj9#20185

Merged

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 2 times, most recently from 88e0beb to 65d97f3 Compare September 19, 2024 20:32

matthewhall2 mentioned this pull request Sep 24, 2024

Define fmax/fmin/dmax/dmin opcode specifications #7293

Open

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 2 times, most recently from 0c276e5 to 9670bf9 Compare September 25, 2024 16:07

matthewhall2 marked this pull request as ready for review September 25, 2024 17:34

matthewhall2 requested review from fjeremic and vijaysun-omr as code owners September 25, 2024 17:34

matthewhall2 mentioned this pull request Sep 25, 2024

Fix fmin/fmax/dmin/dmax node simplifier for const 0s #7471

Merged

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 3 times, most recently from 2f62f58 to 15a2d2b Compare September 27, 2024 19:18

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 5 times, most recently from db6022b to ccc837d Compare October 4, 2024 20:42

matthewhall2 changed the title ~~Support IEEE-754 w.r.t fmin/fmax/dmin/dmax~~ Support IEEE-754 for fmin/fmax/dmin/dmax nodes Oct 4, 2024

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from ccc837d to 9ec2ebd Compare October 4, 2024 20:45

r30shah suggested changes Oct 7, 2024

View reviewed changes

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from 9ec2ebd to 4c079e5 Compare October 8, 2024 14:13

matthewhall2 requested a review from r30shah October 8, 2024 14:13

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from 4c079e5 to d1af547 Compare October 8, 2024 14:15

r30shah reviewed Oct 8, 2024

View reviewed changes

r30shah suggested changes Oct 8, 2024

View reviewed changes

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 3 times, most recently from 3511fe1 to cc9736b Compare October 10, 2024 13:25

Support enabling the inlining of OpenJ9 fmax/fmin/dmax/dmin nodes

476b604

Add Math.max/min/F/D to list of recognized Java methods to: - Disable special aliasing rules since they are intrinsically optimized - Disable implicit asynchronous checks Signed-off-by: Sarwat Shaheen <[email protected]>

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from cc9736b to f82bf4a Compare October 10, 2024 13:37

hzongaro requested review from hzongaro and removed request for fjeremic and vijaysun-omr October 10, 2024 14:03

hzongaro reviewed Oct 10, 2024

View reviewed changes

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from f82bf4a to c925738 Compare October 10, 2024 20:37

matthewhall2 requested a review from hzongaro October 10, 2024 20:47

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 2 times, most recently from 581db9b to d54e28e Compare October 11, 2024 13:30

hzongaro reviewed Oct 11, 2024

View reviewed changes

compiler/z/codegen/ControlFlowEvaluator.cpp Outdated Show resolved Hide resolved

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch 2 times, most recently from 9bd6113 to d3aff89 Compare October 11, 2024 18:21

matthewhall2 requested a review from hzongaro October 11, 2024 18:22

hzongaro reviewed Oct 15, 2024

View reviewed changes

compiler/z/codegen/ControlFlowEvaluator.cpp Outdated Show resolved Hide resolved

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from d3aff89 to da17b3f Compare October 15, 2024 19:18

matthewhall2 requested review from hzongaro and r30shah October 16, 2024 15:26

matthewhall2 force-pushed the fmin_fmax_dmin_dmax branch from da17b3f to fefb344 Compare October 16, 2024 18:37

r30shah approved these changes Oct 17, 2024

View reviewed changes

hzongaro approved these changes Oct 21, 2024

View reviewed changes

hzongaro merged commit ad12524 into eclipse-omr:master Oct 21, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support IEEE-754 for fmin/fmax/dmin/dmax nodes #7464

Support IEEE-754 for fmin/fmax/dmin/dmax nodes #7464

matthewhall2 commented Sep 18, 2024 •

edited

Loading

matthewhall2 commented Oct 4, 2024

r30shah Oct 4, 2024

r30shah Oct 7, 2024

r30shah Oct 7, 2024

r30shah Oct 7, 2024

r30shah Oct 7, 2024

r30shah Oct 7, 2024

r30shah Oct 7, 2024

r30shah Oct 8, 2024

r30shah Oct 8, 2024

r30shah left a comment

r30shah Oct 8, 2024

r30shah commented Oct 8, 2024

matthewhall2 commented Oct 8, 2024 •

edited

Loading

matthewhall2 commented Oct 9, 2024

hzongaro left a comment

hzongaro left a comment

r30shah left a comment

r30shah commented Oct 18, 2024

hzongaro left a comment

		{
		compareRROp = node->getOpCode().isDouble() ? TR::InstOpCode::CDBR : TR::InstOpCode::CEBR;

Support IEEE-754 for fmin/fmax/dmin/dmax nodes #7464

Support IEEE-754 for fmin/fmax/dmin/dmax nodes #7464

Conversation

matthewhall2 commented Sep 18, 2024 • edited Loading

matthewhall2 commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah commented Oct 8, 2024

matthewhall2 commented Oct 8, 2024 • edited Loading

matthewhall2 commented Oct 9, 2024

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro left a comment

Choose a reason for hiding this comment

r30shah left a comment

Choose a reason for hiding this comment

r30shah commented Oct 18, 2024

hzongaro left a comment

Choose a reason for hiding this comment

matthewhall2 commented Sep 18, 2024 •

edited

Loading

matthewhall2 commented Oct 8, 2024 •

edited

Loading