Define fmax/fmin/dmax/dmin opcode specifications

[r30shah]

I wanted to start a discussion around fmax/min and dmax/min IL Opcodes in OMR [1][2]. While working on one accelerating performance of sequence we would generate for code in OpenJ9 where we could leverage such opcodes, we did run into questions involving the specifications for such opcode. Especially how would we handle special cases where operand are signed zero or NaN values. [3]. OMR do not formally define specification for such opcode.

Should we follow IEEE-754 standard for these operations to standardize how to treat NaN / signed Zero? One concern with that would be the difference in how Java treats max / min operations and treats NaN value.

[1].

omr/compiler/il/OMROpcodes.enum

Lines 7944 to 7975 in 6c6d638

	OPCODE_MACRO(\
	/* .opcode = */ fmax, \
	/* .name = */ "fmax", \
	/* .properties1 = */ 0, \
	/* .properties2 = */ ILProp2::ValueNumberShare | ILProp2::SupportedForPRE | ILProp2::Max, \
	/* .properties3 = */ 0, \
	/* .properties4 = */ 0, \
	/* .dataType = */ TR::Float, \
	/* .typeProperties = */ ILTypeProp::Size_4 | ILTypeProp::Floating_Point, \
	/* .childProperties = */ TWO_SAME_CHILD(TR::Float), \
	/* .swapChildrenOpCode = */ TR::BadILOp, \
	/* .reverseBranchOpCode = */ TR::BadILOp, \
	/* .booleanCompareOpCode = */ TR::BadILOp, \
	/* .ifCompareOpCode = */ TR::BadILOp, \
	/* .description = max of 2 or more floats */ \
	)
	OPCODE_MACRO(\
	/* .opcode = */ dmax, \
	/* .name = */ "dmax", \
	/* .properties1 = */ 0, \
	/* .properties2 = */ ILProp2::ValueNumberShare | ILProp2::SupportedForPRE | ILProp2::Max, \
	/* .properties3 = */ 0, \
	/* .properties4 = */ 0, \
	/* .dataType = */ TR::Double, \
	/* .typeProperties = */ ILTypeProp::Size_8 | ILTypeProp::Floating_Point, \
	/* .childProperties = */ TWO_SAME_CHILD(TR::Double), \
	/* .swapChildrenOpCode = */ TR::BadILOp, \
	/* .reverseBranchOpCode = */ TR::BadILOp, \
	/* .booleanCompareOpCode = */ TR::BadILOp, \
	/* .ifCompareOpCode = */ TR::BadILOp, \
	/* .description = max of 2 or more doubles */ \
	)

[2].

omr/compiler/il/OMROpcodes.enum

Lines 8040 to 8071 in 6c6d638

	OPCODE_MACRO(\
	/* .opcode = */ fmin, \
	/* .name = */ "fmin", \
	/* .properties1 = */ 0, \
	/* .properties2 = */ ILProp2::ValueNumberShare | ILProp2::SupportedForPRE | ILProp2::Min, \
	/* .properties3 = */ 0, \
	/* .properties4 = */ 0, \
	/* .dataType = */ TR::Float, \
	/* .typeProperties = */ ILTypeProp::Size_4 | ILTypeProp::Floating_Point, \
	/* .childProperties = */ TWO_SAME_CHILD(TR::Float), \
	/* .swapChildrenOpCode = */ TR::BadILOp, \
	/* .reverseBranchOpCode = */ TR::BadILOp, \
	/* .booleanCompareOpCode = */ TR::BadILOp, \
	/* .ifCompareOpCode = */ TR::BadILOp, \
	/* .description = min of 2 or more floats */ \
	)
	OPCODE_MACRO(\
	/* .opcode = */ dmin, \
	/* .name = */ "dmin", \
	/* .properties1 = */ 0, \
	/* .properties2 = */ ILProp2::ValueNumberShare | ILProp2::SupportedForPRE | ILProp2::Min, \
	/* .properties3 = */ 0, \
	/* .properties4 = */ 0, \
	/* .dataType = */ TR::Double, \
	/* .typeProperties = */ ILTypeProp::Size_8 | ILTypeProp::Floating_Point, \
	/* .childProperties = */ TWO_SAME_CHILD(TR::Double), \
	/* .swapChildrenOpCode = */ TR::BadILOp, \
	/* .reverseBranchOpCode = */ TR::BadILOp, \
	/* .booleanCompareOpCode = */ TR::BadILOp, \
	/* .ifCompareOpCode = */ TR::BadILOp, \
	/* .description = min of 2 or more doubles */ \
	)

[3]. #7266 (comment)

[r30shah]

@0xdaryl I am not sure this is worthy of OMR architecture discussion, but I think atleast we should have some discussions.

FYI @sarwat12 / @hzongaro

[0xdaryl]

In my opinion, the default semantics of the OMR IL opcodes should be as broadly applicable to other language environments as possible. In this particular case, I'd say IEEE 754 semantics fits the bill. Variations in the IL opcode semantics in different language environments can be handled with frontend queries during operations such as constant folding, simplification, or code generation to do the right thing based on the requirements of a particular language. The frontend queries in OMR will answer all the questions as if it were following IEEE 754 semantics, and Java would potentially provide different answers.

[matthewhall2]

@r30shah @0xdaryl @hzongaro While testing this PR for this issue I came across a bug in fmaxminSimplifier() and dmaxminSimplifier() in OMRSimplifierHandlers.cpp: both of these functions handle +/-0 incorrectly. When both args are float/double literals and are equal, fmax/dmax folds to the second argument (does not work for fmax/dmax(+0, -0), and fmin/dmin folds to the first argument (does not work for fmin/dmin(+0, -0). A fix will be implemented in the PR above.

As for the NaNs, the current simplifier works for the Java spec (folds to the first arg, unchanged, if it's a NaN, or the second arg, unchanged, if only the second arg is a NaN), but this does not comply with IEEE-754. An OpenJ9 specific fmax/dmax/fmin/dmin simplifier is needed to get OMR up to the IEEE-754 standard.

Current simplifier for reference:

TR::Node *fmaxminSimplifier(TR::Node * node, TR::Block * block, TR::Simplifier * s)
   {
   s->simplifyChildren(node, block);

   TR::Node * firstChild = node->getFirstChild();
   TR::Node * secondChild = node->getSecondChild();
   bool isBothConst = firstChild->getOpCode().isLoadConst() && secondChild->getOpCode().isLoadConst();
   float fmin = 0, fmax = 0;
   bool maxOpcode = node->getOpCodeValue() == TR::fmax;

   if (isBothConst)
      {
      if (isNaNFloat(firstChild))
         fmin = fmax = firstChild->getFloat();
      else if (isNaNFloat(secondChild))
         fmin = fmax = secondChild->getFloat();
      else
         {
         if (firstChild->getFloat() <= secondChild->getFloat())
            {
            fmin = firstChild->getFloat();
            fmax = secondChild->getFloat();
            }
         else
            {
            fmin = secondChild->getFloat();
            fmax = firstChild->getFloat();
            }
         }
      foldFloatConstant(node, maxOpcode ? fmax : fmin, s);
      }

   return node;
   }

[r30shah]

@matthewhall2 As a first step to that, can we fix the OMRSimplifier to treat +0 and -0 correctly, this will allow you to get your J9 Tests to finish.

For OMR side fix, eventually we need to fix NAN case in the OMR simplifier and extend the function in OpenJ9 that behaves as Java specification.