Ensure simplifier keeps required DIVCHK operations

[hzongaro]

The divchkSimplifier was first simplifying its child node and then checking whether the DIVCHK is still needed after having simplified its child. It did that by checking whether the simplification of its child results in a node that is not the original child, or the simplified child is not a division or remainder operation.

However, it might be that the simplified child is still a division or remainder operation that needs to be checked for division by zero, but is not the original child node. The existing logic leads divchkSimplifier to replace the DIVCHK with a treetop operation incorrectly.

This change revises the division and remainder simplifiers so that they only directly replace the child of the parent DIVCHK (if any) with a different division or remainder node that must still be checked for division by zero.

This pull request also contains a minor change to a trace message in OMR::Optimization::anchorChildren.

Fixes: eclipse-openj9/openj9#17066

[hzongaro]

Devin @jdmpapin, may I ask you to review this change?

I have to admit that I'm a little bit uncomfortable with the handshake that exists between the DIVCHK simplifier and the ldiv and lrem simplifiers, but I didn't see another obvious way of handling this situation. I'm open to suggestions for cleaner approaches to dealing with this problem.

[jdmpapin]

In terms of the handshake, I think it would be better to be more explicit. (edit: I mean in the code itself. Your comments are very explicit already!) Here's an idea:

1. Add a field to Simplifier to be this side-channel, e.g. TR::Node *_newDivCheckNode;
2. Have the DIVCHK simplifier handler check to see whether the child has been simplified yet. If so, then it was evaluated above, so eliminate the DIVCHK
3. If the child hasn't been simplified yet in this pass but is somehow already not a div/mod anyway, eliminate the DIVCHK - I'm not sure if this is possible, but it seems prudent to check for it
4. Otherwise, simplify the child like we currently do: s->simplify(child, block)
5. Make the *div and *rem handlers set the _newDivCheckNode, but only after recursively simplifying their own children, and make them stop mutating the parent DIVCHK directly
6. The child of DIVCHK will therefore be the last to set _newDivCheckNode, so the DIVCHK handler can consult it

I imagine it would be treated something like the following: the DIVCHK handler ignores the result of simplify() and uses _newDivCheckNode instead. If it's null, eliminate the DIVCHK. Otherwise, replace the child. (It could just be the existing child, in which case the replacement does nothing.) I think it would be pretty easy to understand what it means to set _newDivCheckNode to a particular value, though it would certainly take a comment in the DIVCHK handler to explain why it is that we know that _newDivCheckNode has been set while simplifying the child rather than a more distant descendant or some other node from a prior tree

What do you think?

[hzongaro]

What do you think?

Thanks for the suggestion, Devin @jdmpapin! Let me play with that. I'll move this pull request back to draft status in the meanwhile.

[hzongaro]

Devin @jdmpapin, as you suggested I revised the changes to have the {l|i}{rem|div}Simplifiers use a field — _nodeToDivchk — in OMR::Simplifier to communicate to divchkSimplifier whether there is a node that still needs to have a DIVCHK applied to it following any simplification that might have happened. May I ask you to review this change again?

[hzongaro]

Devin @jdmpapin, when you have the opportunity, may I ask you to review the revised changes for this pull request?

[jdmpapin]

My biggest suggestion is the comment I left in idivSimplifier(), which I think would significantly simplify the logic and perhaps also eliminate the need to change fold*Constant() to indicate success. My only reservation is that by not bothering to precisely distinguish the cases where eliminating DIVCHK is optional vs. mandatory, we'd lose the ability to performTransformation() in the optional cases. But I'm not sure how concerned to be about that, and if we decide it's important, then I think we could do an earlier performTransformation() that covers more logic, e.g. something along the lines of

if (divisor != 0 && performTransformation(...))

[hzongaro]

Thanks for your review, Devin @jdmpapin. I took your suggestion and performed an early call to performTransformation in the various simplifiers to control whether to set _nodeToDivchk to NULL, effectively controlling whether the DIVCHK can be removed and whether its child can be simplified. With that, I was able to revert the changes to have the various fold*Constant methods return a boolean value, as you had observed.

Once you're OK with these changes, I will squash commits 37c05e2 and 7ebe636.

The one comment I've left open is on BINARY_IDENTITY_OP.

May I ask you to re-review?

[jdmpapin]

Thanks for the update! I have a few more comments

[hzongaro]

I agree it would be better for BINARY_IDENTITY_OP() to become a function. . . .

Whether we change it to a function or leave it as-is, it might be better to move the test for a constant firstChild to a nested if that guards just the constant folding, and the BINARY_IDENTITY_OP() part could move into the outer if

Yes, that would be better than bulking up the definition of BINARY_IDENTITY_OP(). I'll leave it as a macro for now, and I'll open a follow up issue to change it into a function.

[hzongaro]

Devin @jdmpapin, this change is ready for another review. Again, once approved, I will squash commits 04b2b82, 37c05e2 and 7ebe636.

[jdmpapin]

Thanks for all the updates! LGTM, with one small oddity

[jdmpapin]

Awesome, good to squash!

[hzongaro]

Thanks for your thorough reviews and the original suggestion on how to handle the handshake, Devin @jdmpapin. I've squashed the commits.

[jdmpapin]

Jenkins build all

[hzongaro]

I think the x86-64 macOS failure is known issue #6516.

[jdmpapin]

All other tests have passed