Max and Min IL opcodes should have two children

[0xdaryl]

Ensure the IL opcodes

TR::imax
TR::imin
TR::lmax
TR::lmin

only support two children. Some optimizer and code generator support (see references below for a couple of examples) assume that these opcodes could have more than two children, and provides support for that case. These cases should be found and removed.

See redacted text below for background details.

The number of children of IL opcodes:

TR::imax
TR::imin
TR::lmax
TR::lmin

~~is handled inconsistently. The IL opcode properties table [1] specifies the opcodes behave like other binary operators and have only two children. However, other code like [2][3] suggests it can have a variable number of children.~~

~~I suspect the variable nature is historical (pre-open source). Nothing in the current code base nor any known downstream project generates these opcodes with more than two children.~~

~~I propose to make these opcodes consistent with other operators and enforce the two child property. Code that supports multiple children should be cleaned up.~~

~~These opcodes are somewhat unique in that they are reduction operations which might justify more than two children. However, they are ultimately reduced pairwise by the backends anyway. I'm not sure the value in allowing more than two children is there.~~

@gita-omr @vijaysun-omr @andrewcraik @fjeremic @leonardo2718 @knn-k : please share any opinions.

[1] https://github.com/eclipse/omr/blob/b9ba1f63eafc8b7f4e80c47a0893f16dd8ffdbab/compiler/il/OMRILOpCodeProperties.hpp#L11719 [2] https://github.com/eclipse/omr/blob/b9ba1f63eafc8b7f4e80c47a0893f16dd8ffdbab/compiler/optimizer/OMRSimplifierHandlers.cpp#L16957 [3] https://github.com/eclipse/omr/blob/b9ba1f63eafc8b7f4e80c47a0893f16dd8ffdbab/compiler/p/codegen/ControlFlowEvaluator.cpp#L4171

[andrewcraik]

Limiting to the binary case makes sense to me. If you want to do multiple then some kind of vector max/min would seem to be the right representation rather than a variable number of children.

[fjeremic]

I agree to limiting to only the two child versions. @dchopra001 for any additional input given your expertise in the area.

[gita-omr]

Could we find out who creates those nodes with more than two children? Sorry, I don’t have access to the source right now.

[0xdaryl]

@gita-omr, we didn't find any in OMR or OpenJ9. We only found code in the optimizer and some backends to handle more than 2 children.

[AidanHa]

Will be tackling this issue. Should we also be changing iumax, lumax, fmax and dmax?

[0xdaryl]

I will cautiously say yes. All of those opcodes are also specified as having two children in the OMR ILOpCodeProperties table. You should check through the source code (OMR and OpenJ9) to see where they are generated (i.e., either in IL generator or through an optimizer pass like simplifier) and where they are actually manipulated to see if there is any code that assumes there are more than two children expected. The reasoning for restricting those opcodes to two children is the same as the original opcodes in this issue. I'll let others chime in if they have a difference of opinion.

[dchopra001]

I just noticed something strange on Z. We assign all the *max nodes to the same evaluator here: https://github.com/eclipse/omr/blob/14248fb2342a51067ce38921f34aeda8dfb7cc53/compiler/z/codegen/OMRTreeEvaluatorTable.hpp#L809-L814

The OMR::Z::TreeEvaluator::maxEvaluator uses the following helper routine: https://github.com/eclipse/omr/blob/14248fb2342a51067ce38921f34aeda8dfb7cc53/compiler/z/codegen/ControlFlowEvaluator.cpp#L343-L351

This helper routine doesn't handle fmax nodes. I'm surprised we haven't seen this in a test failure yet as we should always hit the assert at the end of this routine. Either that or we never generate fmax nodes. Similar issue applies to fmin and dmin nodes.

[0xdaryl]

I don't believe any ILGen or optimization pass currently generates the TR::[fd]max or TR::[fd]min opcodes.

[fjeremic]

I don't believe any ILGen or optimization pass currently generates the TR::[fd]max or TR::[fd]min opcodes.

This is the correct answer. As part of this work I'd like to see some Tril tests against these to weed out such bugs.

[dchopra001]

To stay consistent, shouldn't we also change TR::TreeEvaluator::maxEvaluator, // TR::fmax to TR::TreeEvaluator::UnImpOpEvaluator, // TR::fmax?

(For Z at least. On Power the evaluator handles floating point opcodes already)

[AidanHa]

To stay consistent, shouldn't we also change TR::TreeEvaluator::maxEvaluator, // TR::fmax to TR::TreeEvaluator::UnImpOpEvaluator, // TR::fmax?

(For Z at least. On Power the evaluator handles floating point opcodes already)

@dchopra001 Should this be the same for fmin, dmax and dmin?

[dchopra001]

@dchopra001 Should this be the same for fmin, dmax and dmin?

Yes, since those are unimplemented as well.

[0xdaryl]

Re-opening as #4259 hasn't been merged yet.

[github-actions[bot]]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment on the issue or it will be closed in 60 days.

[Leonardo2718]

This is being worked on so it's not really stale.