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

matthewhall2 · 2024-09-25T19:41:39Z

The fmaxminSimplifier and dmaxminSimplifier methods were returning the wrong value when given (+0, -0). This change handles +-0 edge case to return the 0 with the correct sign (+0 is strictly greater than -0).

Supports change in #7464 and eclipse-openj9/openj9#20185 (needed for 20185 to pass)
@r30shah

r30shah · 2024-09-25T20:25:14Z

compiler/optimizer/OMRSimplifierHandlers.cpp

-            fmax = secondChild->getFloat();
+            if (first == second && first == 0)
+               {
+               int firstBits = *(int *)(&first);


Can we use reinterpret_cast here?

actually, I believe its not guaranteed that float is 32 bits or, more importantly, that int and float are the same size right (same for double and long or long long)? Do we have an int type that's typedefed or #defined so that its the same size as float, or do I have to add some more logic to work with the proper size?

You did change the reinterpret_cast to use int32_t right, that would use 4 bytes for int32_t. I think it is fair to assume that float will be binary floating point usually following IEEE-754 binary32 format. You can also call node->getFloatBits that would return bits representation of floating point value

Also I believe similar simplification for Postive and Negative floats were done in fdivSimplifier in same file, you can take a look at that as well to see how special case negative / positive zero is coded, also checkout [1]

[1]. https://github.com/eclipse/omr/blob/master/compiler/il/OMRDataTypes.hpp

r30shah · 2024-09-25T20:27:21Z

compiler/optimizer/OMRSimplifierHandlers.cpp

+               {
+               int firstBits = *(int *)(&first);
+               int secondBits = *(int *)(&second);
+               int sign = firstBits & nZeroBits;


Do we need to. zero out remaining bits here ?

shouldn't need to right? the bitwise And with nZeroBits should zero all but the most significant bit

I meant, if the float is negative 0, can that be represented by some other bit pattern other than 0x80000000 ?

There should only be 1 representation for -0 for all floating point representations (whether it's binary interchange format or decimal interchange format [both for decimal and binary encoding formats of the significand]). If the system uses fixed-point as the underlying type of float then there is no -0, and 0 is just 0x0 which should still work

r30shah · 2024-09-25T20:34:59Z

compiler/optimizer/OMRSimplifierHandlers.cpp

            {
-            fmin = firstChild->getFloat();
-            fmax = secondChild->getFloat();
+            if (first == second && first == 0)


Wondering, if GCC or other compiler adheres to IEE-754, then this condition would be incorrect right ?

ah yes in that case it would not work. Changed it to compare as ints instead and put as first if condition (no nesting)

did you mean if other compilers do NOT adhere to IEEE-754? 0 == -0 is true for IEEE

So comparison of +0/-0 should return true if compiler is following IEEE-754. I asked the question to confirm this.

Have not worked out on edge cases, can following check work ?

if (first > second || (firstBits == pZeroBits && secondBits == nZeroBits)) fmax = first fmin = second else // first < second, first == second , (first == nZeroBits && second dontCare) fmax = second fmin = first

yup that'll work. Still need the additional checks for NaNs though, since both max and min need to evaluate to the same NaN

I think the previous implementation before your fix did that.

r30shah · 2024-09-25T20:35:13Z

compiler/optimizer/OMRSimplifierHandlers.cpp

-            max = secondChild->getDouble();
+            if (first == second && first == 0)
+               {
+               long long firstBits = *(long *)(&first);


Similar comments as Floats

r30shah · 2024-09-26T14:22:22Z

compiler/optimizer/OMRSimplifierHandlers.cpp

+         float second = secondChild->getFloat();
+         int32_t firstBits = *reinterpret_cast<int32_t *>(&first);
+         int32_t secondBits = *reinterpret_cast<int32_t *>(&second);
+         int32_t nZeroBits = 0x80000000;


We do not need to use variable to hold this values (bits representation of negative and positive 0.0F).

r30shah · 2024-09-26T14:24:55Z

compiler/optimizer/OMRSimplifierHandlers.cpp

+         if ((firstBits == pZeroBits || firstBits == nZeroBits) && (secondBits == pZeroBits || secondBits == nZeroBits))
+            {
+            int32_t sign = firstBits & nZeroBits;
+            fmin = sign < 0 ? first : second;


Couple of thoughts here, do we really need to know sign here? Also I feel we can study the cases and can achieve what we want to in two conditional block only (We can try to avoid if/else if/else if possible).

r30shah · 2024-09-26T14:25:15Z

compiler/optimizer/OMRSimplifierHandlers.cpp

+         double second = secondChild->getDouble();
+         int64_t  firstBits = *reinterpret_cast<int64_t *>(&first);
+         int64_t  secondBits = *reinterpret_cast<int64_t *>(&second);
+         int64_t  nZeroBits = 0x8000000000000000L;


Similar comments as above.

r30shah · 2024-09-26T19:29:29Z

compiler/optimizer/OMRSimplifierHandlers.cpp

+   uint32_t firstBits = firstChild->getFloatBits();
+   uint32_t secondBits = secondChild->getFloatBits();
+
+   if (first != first) // this is true only for NaNs


I understand this comparison that you did, but actually prefer what we had originally. That is more readable and checks bit pattern as well.

would it be better to change the implementation of isNaNFloat and isDoubleFloat to use value != value? Or is there some other reason why we check the bit pattern?

Yes, as I said, I understand value != value will be true for NaN, I think the explicit check is the preferred way, as if you go through other compiler documentation, it says you can use it but explicit is the preferred way. Also we would be in near future look into this code for NaNs so we should try to address that in other PR and keep this PR limited to what we want to fix.

r30shah

Last nitpick, seems OK to me

r30shah · 2024-09-26T20:20:27Z

compiler/optimizer/OMRSimplifierHandlers.cpp

+      }
+   else if (first > second || (firstBits == 0 && secondBits == FLOAT_NEG_ZERO))
+      {
+         fmax = first;


Extra indentation here and at other places.

r30shah

LGTM.

r30shah · 2024-09-26T20:36:08Z

jenkins build all

matthewhall2 · 2024-09-27T16:37:41Z

seems like the git clones all failed @r30shah ? Can you rerun please?

r30shah · 2024-09-27T16:52:54Z

jenkins build all

r30shah · 2024-09-30T19:45:26Z

@hzongaro Can I request your review on this change and if you are OK with it, can you merge this change as well?

hzongaro

I think the changes look good. I just had a suggestion for a comment that might be helpful.

Also, may I ask you to adjust the commit comment per the Commit Guidelines? In particular, lines in the body should be no more than 72 colums long, and the header should use imperative to describe the change - perhaps something like, Correct simplification of signed zero for floating point min and max

compiler/optimizer/OMRSimplifierHandlers.cpp

- fmax/dmax(+0, -0) now folds to +0, fmin/dmin(+0, -0) to -0 - (previous version returned 2nd arg on fmax/dmax nodes and 1st arg on fmin/dmin nodes Signed-off-by: Matthew Hall <[email protected]>

hzongaro

Looks good. Thanks!

hzongaro · 2024-10-02T20:44:22Z

The latest updates were only to comments and previous test runs were successful, so there's no need to rerun testing. Merging.

github-actions bot added the comp:compiler label Sep 25, 2024

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch from 823c8ed to 1def696 Compare September 25, 2024 19:43

matthewhall2 marked this pull request as ready for review September 25, 2024 19:45

matthewhall2 requested a review from vijaysun-omr as a code owner September 25, 2024 19:45

matthewhall2 mentioned this pull request Sep 25, 2024

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

Merged

matthewhall2 changed the title ~~Fixed fmin/fmax/dmin/dmax node simplifier for const 0s~~ Fix fmin/fmax/dmin/dmax node simplifier for const 0s Sep 25, 2024

r30shah suggested changes Sep 25, 2024

View reviewed changes

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch 2 times, most recently from 0aa4f79 to d044d58 Compare September 25, 2024 21:44

matthewhall2 requested a review from r30shah September 25, 2024 21:50

r30shah suggested changes Sep 26, 2024

View reviewed changes

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch from d044d58 to cdaab4c Compare September 26, 2024 19:08

matthewhall2 requested a review from r30shah September 26, 2024 19:09

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch 2 times, most recently from 2a24202 to dfec87e Compare September 26, 2024 19:22

r30shah suggested changes Sep 26, 2024

View reviewed changes

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch from dfec87e to 5947bbd Compare September 26, 2024 20:12

matthewhall2 requested a review from r30shah September 26, 2024 20:12

r30shah approved these changes Sep 26, 2024

View reviewed changes

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch from 5947bbd to 40b9623 Compare September 26, 2024 20:25

matthewhall2 requested a review from r30shah September 26, 2024 20:25

r30shah approved these changes Sep 26, 2024

View reviewed changes

hzongaro self-assigned this Sep 30, 2024

hzongaro self-requested a review September 30, 2024 19:51

hzongaro reviewed Sep 30, 2024

View reviewed changes

compiler/optimizer/OMRSimplifierHandlers.cpp Show resolved Hide resolved

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch from 40b9623 to 3b51547 Compare October 1, 2024 19:13

Correct simplification of signed-0 for floating point min and max

be71516

- fmax/dmax(+0, -0) now folds to +0, fmin/dmin(+0, -0) to -0 - (previous version returned 2nd arg on fmax/dmax nodes and 1st arg on fmin/dmin nodes Signed-off-by: Matthew Hall <[email protected]>

matthewhall2 force-pushed the fdminmax_simplifier_zeros branch from 3b51547 to be71516 Compare October 1, 2024 19:21

matthewhall2 requested a review from hzongaro October 1, 2024 19:22

hzongaro approved these changes Oct 2, 2024

View reviewed changes

hzongaro merged commit d4f0498 into eclipse-omr:master Oct 2, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

matthewhall2 commented Sep 25, 2024 •

edited

Loading

r30shah Sep 25, 2024

matthewhall2 Sep 26, 2024

r30shah Sep 26, 2024

r30shah Sep 26, 2024 •

edited

Loading

r30shah Sep 25, 2024

matthewhall2 Sep 25, 2024

r30shah Sep 26, 2024

matthewhall2 Sep 26, 2024

r30shah Sep 25, 2024

matthewhall2 Sep 25, 2024

matthewhall2 Sep 26, 2024

r30shah Sep 26, 2024

r30shah Sep 26, 2024

matthewhall2 Sep 26, 2024

r30shah Sep 26, 2024

r30shah Sep 25, 2024

r30shah Sep 26, 2024

r30shah Sep 26, 2024

r30shah Sep 26, 2024

r30shah Sep 26, 2024

matthewhall2 Sep 26, 2024 •

edited

Loading

r30shah Sep 26, 2024 •

edited

Loading

r30shah left a comment

r30shah Sep 26, 2024

r30shah left a comment

r30shah commented Sep 26, 2024

matthewhall2 commented Sep 27, 2024

r30shah commented Sep 27, 2024

r30shah commented Sep 30, 2024

hzongaro left a comment

hzongaro left a comment

hzongaro commented Oct 2, 2024

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

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

Conversation

matthewhall2 commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah Sep 26, 2024 • edited Loading

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

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

matthewhall2 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

r30shah Sep 26, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

r30shah commented Sep 26, 2024

matthewhall2 commented Sep 27, 2024

r30shah commented Sep 27, 2024

r30shah commented Sep 30, 2024

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro left a comment

Choose a reason for hiding this comment

hzongaro commented Oct 2, 2024

matthewhall2 commented Sep 25, 2024 •

edited

Loading

r30shah Sep 26, 2024 •

edited

Loading

matthewhall2 Sep 26, 2024 •

edited

Loading

r30shah Sep 26, 2024 •

edited

Loading