Refactor Binary Identity Simplification Macro

[jmesyou]

This commit cleans up the BINARY_IDENTITY_OP and
BIANRY_IDENTITY_OR_ZERO macros and replaces them
with inline equivalents by implementing a struct to replace template
instantions of the functions tryAndSimplifyBinaryOp*.

This change limits the number of template instantiations
potential call sites have to create as well as abstracts
some captured variables inside the struct to minimize
the number of arguments passed to actual calls.

A closure is emulated by the struct.
Through escape analysis, scalar replacement and inlining,
the overhead of creating this struct should be minimal
on -O3

Related: #7166

Signed-off-by: James You [email protected]

[hzongaro]

It looks like there are some build errors on Windows and macOS.

D:\a\1\s\compiler\optimizer\OMRSimplifierHandlers.cpp(6550,24): error C2672: 'tryAndSimplifyBinaryIdentityOp': no matching overloaded function found [D:\a\1\s\build\jitbuilder\jitbuilder.vcxproj]
D:\a\1\s\compiler\optimizer\OMRSimplifierHandlers.cpp(6550,7): error C2782: 'TR::Node *tryAndSimplifyBinaryIdentityOp(TR::Simplifier *,T (__cdecl *)(TR::Node *),TR::Node *,T)': template parameter 'T' is ambiguous [D:\a\1\s\build\jitbuilder\jitbuilder.vcxproj]
D:\a\1\s\compiler\optimizer\OMRSimplifierHandlers.cpp(249): message : see declaration of 'tryAndSimplifyBinaryIdentityOp' [D:\a\1\s\build\jitbuilder\jitbuilder.vcxproj]

[jmesyou]

@hzongaro ready for another look!

[hzongaro]

@a7ehuo, I haven't managed to spend the time needed to review this pull request properly. May I ask you to review?

[a7ehuo]

@jmesyou Does the second commit almost rewrite the core change made in the first commit? It might be hard to review since any comment added to the first commit might not be applicable any more after the second commit. And some of the comments might need to refer to the code in the first commit but the suggestion would apply to the second commit since the structure of the design has been changed in the second commit. Could the two commits be squashed into one to make it easier for review? Thank you!

Could the commit comment and the PR description be updated to reflect more accurately on the content since this PR doesn't solve the whole issue but it addresses 2 of the 3 macros mentioned in the issue #7166

Fixes: #7166
=>
Related: #7166

[jmesyou]

@a7ehuo I squashed the commits together. Hopefully that will make the PR and changes easier to read. I've also updated the Fixes messaged to the Related message.

[a7ehuo]

Thank you very much for making this change to clean up the code and squashing the two commits! I just have some minor comments.

Meanwhile could the description of this PR be updated with the latest commit message? I'd also recommend running some regression tests such as sanity.functional, extended.functional, or sanity.system, sanity.openjdk etc. on different platforms with this change to make sure there is no regression.

[a7ehuo]

Thank you very much for making the update @jmesyou! I just have some minor follow up comments.

[a7ehuo]

Thanks for making the update @jmesyou! I have two comments as below

[a7ehuo]

Thank you for the update!

LGTM. I assume you're running some regression tests (sanity.functional, extended.functional, sanity.openjdk etc). Once you confirm the regression tests complete successfully and all the commits are squashed into one. I'll approve the PR. Thanks again!

[jmesyou]

Internal build 21052 is passing and commits squashed

[a7ehuo]

LGTM . Thank you very much for making the update @jmesyou!

[hzongaro]

I think the changes look good. I have a number of cosmetic comments, mostly on the existing code.

[hzongaro]

Thanks for the updates. All your changes look good - just one further comment. Once addressed, you can go ahead and squash the commits. Thanks!

[hzongaro]

Looks good. Thanks!

[hzongaro]

Jenkins build all

[hzongaro]

It looks like the z/OS and AIX compilers are having trouble with part of these changes.

13:34:33  "/home/omr/workspace/Build/compiler/optimizer/OMRSimplifierHandlers.cpp", line 266.38: 1540-0063 (S) The text "s" is unexpected.
13:34:33  "/home/omr/workspace/Build/compiler/optimizer/OMRSimplifierHandlers.cpp", line 271.39: 1540-0063 (S) The text "s" is unexpected.
13:34:33  "/home/omr/workspace/Build/compiler/optimizer/OMRSimplifierHandlers.cpp", line 276.39: 1540-0063 (S) The text "s" is unexpected.
13:34:33  "/home/omr/workspace/Build/compiler/optimizer/OMRSimplifierHandlers.cpp", line 281.39: 1540-0063 (S) The text "s" is unexpected.
13:34:33  "/home/omr/workspace/Build/compiler/optimizer/OMRSimplifierHandlers.cpp", line 286.40: 1540-0063 (S) The text "s" is unexpected.
13:34:33  "/home/omr/workspace/Build/compiler/optimizer/OMRSimplifierHandlers.cpp", line 291.40: 1540-0063 (S) The text "s" is unexpected.

[jmesyou]

@hzongaro had to rebase and force push to run the Jenkins pipeline, sorry about that 😅

[hzongaro]

Jenkins build all

[jmesyou]

RISV failure looks unrelated, had similar failure yesterday on unrelated PR #7287. MacOS failure known to be #7181

[hzongaro]

RISV failure looks unrelated, had similar failure yesterday on unrelated PR #7287

Opened issue #7291 for the RISC-V failure.