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

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

Open
r30shah opened this issue Mar 25, 2024 · 4 comments
Open

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

r30shah opened this issue Mar 25, 2024 · 4 comments

Comments

@r30shah
Copy link
Contributor

r30shah commented Mar 25, 2024

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]. https://github.com/eclipse/omr/blob/6c6d6383ccd93937d1987de8e818a0a63118b01e/compiler/il/OMROpcodes.enum#L7944-L7975
[2]. https://github.com/eclipse/omr/blob/6c6d6383ccd93937d1987de8e818a0a63118b01e/compiler/il/OMROpcodes.enum#L8040-L8071
[3]. #7266 (comment)

@r30shah
Copy link
Contributor Author

r30shah commented Mar 25, 2024

@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
Copy link
Contributor

0xdaryl commented Mar 26, 2024

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
Copy link
Contributor

matthewhall2 commented Sep 24, 2024

@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
Copy link
Contributor Author

r30shah commented Sep 24, 2024

@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.

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

No branches or pull requests

3 participants