Stop generating deprecated unsigned opcodes of equality compare and const #4347

wbh123456 · 2019-09-23T19:47:38Z

Delete any creations or recreations of node with unsigned deprecated IL opcodes in Equality compare/Equality Compare and Branch/Constant category.

Using TR_ASSERT_FATAL to ensure no downstream projects will attempt to use aforementioned opcodes.

Issue: #2657
Signed-off-by: Bohao(Aaron) Wang aaronwang0407@gmail.com

andrewcraik · 2019-09-23T20:24:24Z

Are any changes required in downstream projects before the ASSERT_FATAL is added? We don't want to break consumers of OMR needlessly.

wbh123456 · 2019-09-24T13:22:57Z

Are any changes required in downstream projects before the ASSERT_FATAL is added? We don't want to break consumers of OMR needlessly.

Any creation with Equality compare/Equality Compare and Branch/Constant opcodes in the list of #2657 would need to be removed.

andrewcraik · 2019-09-27T13:14:38Z

@wbh123456 since the fatal assert is in another WIP PR can you remove it from here for now then we can at least merge this change to stop generating some of these deprecated opcodes....

wbh123456 · 2019-09-28T03:20:21Z

@wbh123456 since the fatal assert is in another WIP PR can you remove it from here for now then we can at least merge this change to stop generating some of these deprecated opcodes....

The fatal asserts for this PR is only asserting the generations of the deprecated Equality compare/Equality Compare and Branch/Constant Opcodes, which is only within the scope of this PR.

wbh123456 · 2019-10-02T14:17:46Z

Passed internal testings with OpenJ9 master branch. Should be good for a review. @andrewcraik

andrewcraik · 2019-10-02T19:10:53Z

@genie-omr build all

andrewcraik · 2019-10-02T19:11:36Z

Appveyor build looks to be infra - dependency download failure.

wbh123456 · 2019-10-03T18:19:08Z

The OMR check failures are likely infra errors. @andrewcraik

andrewcraik · 2019-10-03T19:07:20Z

@genie-omr build all

andrewcraik · 2019-10-04T13:23:35Z

Doing a quick search in OpenJ9 as a major consumer of OMR they have at least one place lest that generates a cconst that could fail with the assert added in this commit (https://github.com/eclipse/openj9/blob/d8be80662bce5a7d8dd62b79cd6daf2abd3be9ca/runtime/compiler/optimizer/SequentialStoreSimplifier.cpp#L1777).

wbh123456 · 2019-10-04T14:24:30Z

Doing a quick search in OpenJ9 as a major consumer of OMR they have at least one place lest that generates a cconst that could fail with the assert added in this commit (https://github.com/eclipse/openj9/blob/d8be80662bce5a7d8dd62b79cd6daf2abd3be9ca/runtime/compiler/optimizer/SequentialStoreSimplifier.cpp#L1777).

This cconst is a TR::Node::cconst function which is different from a TR::cconst IL opcode. In addition, I have mapped the cconst creation from sconst to cconst inside the TR::Node::cconst function. There should be no other cconst creations in OpenJ9.

andrewcraik · 2019-10-10T14:15:25Z

compiler/il/OMRNode.hpp

+   static bool isNotDeprecatedUnsigned(TR::ILOpCodes opvalue)
+      {
+
+      TR::ILOpCodes DeprecatedUnsignedList [] = 


per other PRs in your series please convert to a switch statement on the opcode.

Changed the structure of this function in 1a4a32c. Keep the structure consistent with the same function in #4367.
FYI, there are 2 other WIP PRs also introducing the same function using the old structure. I will unWIP those PRs and resolve the conflicts there to update this function once this PR or #4367 has been merged.

wbh123456 · 2019-10-16T13:47:03Z

This PR is good for a review. @Leonardo2718 @andrewcraik @vijaysun-omr

Leonardo2718 · 2019-10-16T14:02:43Z

@genie-omr build all

andrewcraik · 2019-10-16T15:47:41Z

compiler/optimizer/OMRSimplifierHandlers.cpp

@@ -13245,7 +13245,7 @@ TR::Node *ificmpneSimplifier(TR::Node * node, TR::Block * block, TR::Simplifier
      }

   if (node->getOpCodeValue() == TR::ificmpne)
-      intCompareNarrower(node, s, TR::ifsucmpne, TR::ifscmpne, TR::ifbcmpne);
+      intCompareNarrower(node, s, TR::ifscmpne, TR::ifscmpne, TR::ifbcmpne); //ushortOp is the same as shortOp in this case


not sure why this needs a comment - the others don't

Not sure why this comment is there. I don't think I wrote that comment. Do you think it is a good idea to just remove it as it does not give much useful information.

looks like you added it, but regardless it is redundant and should be removed IMO.

andrewcraik · 2019-10-16T16:45:10Z

Modulo the comment issue above - I'm good with this.

Leonardo2718 · 2019-10-16T18:04:01Z

@genie-omr build all

Leonardo2718 · 2019-10-18T15:06:59Z

@genie-omr build all

Leonardo2718 · 2019-10-18T19:36:04Z

@genie-omr build aix

wbh123456 · 2019-10-18T19:57:30Z

Passed all the checks + Passed all the extended internal testings. Should be good for a review. @Leonardo2718 @vijaysun-omr

wbh123456 · 2019-10-31T18:45:21Z

This PR maps the creations of any of the following ILOpcodes into appropriate signed version.
bucmpeq bucmpne iucmpeq iucmpne lucmpeq lucmpne sucmpeq sucmpne
ifiucmpeq ifiucmpne iflucmpeq iflucmpne ifbucmpeq ifbucmpne ifsucmpeq ifsucmpne
buconst iuconst luconst cconst.
(e.g. cconst --> sconst, ifsucmpeq --> ifscmpeq)

Note that there isn't any coordinate delivery to this PR.

Checks that have been done to ensure correctness and performance:

Performed extended internal testings
Fatal asserted any creation or recreations of the above Opcodes
Checked the code path of all uses/references of above opcodes to ensure their mapped signed version would work the same way as the deprecated unsigned version
Checked all associated idiomrecognition units, all mappings performed would not affect its pattern recognized.

FYI @andrewcraik

Leonardo2718 · 2019-11-11T18:28:40Z

@genie-omr build all

Leonardo2718 · 2019-11-27T14:34:45Z

@andrewcraik Given #4604, I'm wondering if we can expedite merging this PR?

andrewcraik · 2019-11-27T14:39:08Z

@wbh123456 I see a merge commit on Nov 11 I don't think we want that in the history can you just rebase the changes up? @Leonardo2718 I'm fine with the change (as indicated by the approved review) once the merge commit is cleaned up. As long as people have checked we aren't breaking OpenJ9 this should be good to go. If there is a change needed to prevent OpenJ9 breaking then we should make sure that is merged first.

wbh123456 · 2019-11-27T18:04:39Z

I have properly rebased the changes in fbf26e2. Sorry for the inconvenience I brought. FYI @andrewcraik @Leonardo2718

andrewcraik · 2019-11-27T18:06:21Z

@genie-omr build all

compiler/optimizer/OMRSimplifierHandlers.cpp

Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

Specifically, remove creation of opcodes in Equality compare and equality compare and branch Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

…equality Add TR_ASSERT_FATAL to ensure there is no more creatation or recreation of node with deprecated opcode in the equality compare and equality compare branch category Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

andrewcraik · 2019-11-27T21:32:34Z

@genie-omr build all

Leonardo2718

LGTM 👍

wbh123456 requested review from andrewcraik, Leonardo2718 and vijaysun-omr as code owners September 23, 2019 19:47

wbh123456 force-pushed the unsigned_gen branch from ae421d9 to c9560db Compare September 23, 2019 19:50

wbh123456 force-pushed the unsigned_gen branch from c9560db to 90cd064 Compare September 23, 2019 20:27

Leonardo2718 added the comp:compiler label Sep 23, 2019

wbh123456 force-pushed the unsigned_gen branch from 90cd064 to e36afd5 Compare September 24, 2019 13:19

wbh123456 mentioned this pull request Sep 24, 2019

Remove deprecated unsigned IL opcodes #2657

Closed

wbh123456 changed the title ~~WIP: Stop generating deprecated unsigned opcodes of equality compare and const~~ Stop generating deprecated unsigned opcodes of equality compare and const Sep 24, 2019

wbh123456 force-pushed the unsigned_gen branch from e36afd5 to 02cf2d8 Compare September 24, 2019 14:34

wbh123456 mentioned this pull request Sep 28, 2019

Stop generating deprecated unsigned opcodes of load and store from OMR #4356

Merged

wbh123456 force-pushed the unsigned_gen branch from 02cf2d8 to 177f177 Compare October 1, 2019 17:38

andrewcraik approved these changes Oct 2, 2019

View reviewed changes

andrewcraik reviewed Oct 10, 2019

View reviewed changes

wbh123456 force-pushed the unsigned_gen branch 2 times, most recently from 1a4a32c to 7d9723e Compare October 10, 2019 19:09

andrewcraik reviewed Oct 16, 2019

View reviewed changes

wbh123456 force-pushed the unsigned_gen branch from 7d9723e to 9100936 Compare October 16, 2019 17:35

wbh123456 force-pushed the unsigned_gen branch from 9100936 to 83b926a Compare October 16, 2019 21:18

wbh123456 mentioned this pull request Oct 22, 2019

Remove references of deprecated const IL Opcode #4033

Merged

knn-k mentioned this pull request Nov 27, 2019

WIP: AArch64: Temporary workaround for iuconstEvaluator #4604

Closed

wbh123456 force-pushed the unsigned_gen branch from 2782668 to fbf26e2 Compare November 27, 2019 18:03

Leonardo2718 reviewed Nov 27, 2019

View reviewed changes

compiler/optimizer/OMRSimplifierHandlers.cpp Outdated Show resolved Hide resolved

wbh123456 added 4 commits November 27, 2019 15:45

Remove creation of nodes with deprecated unsigned Constant opcodes

2e4a572

Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

Remove Creation of nodes with deprecated unsigned Equality opcodes

d3ec5fa

Specifically, remove creation of opcodes in Equality compare and equality compare and branch Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

Fatal assert any creation of nodes with deprecated constant opcodes

14de6f2

Signed-off-by: Bohao(Aaron) Wang <aaronwang0407@gmail.com>

wbh123456 force-pushed the unsigned_gen branch from fbf26e2 to 14de6f2 Compare November 27, 2019 20:46

Leonardo2718 approved these changes Nov 27, 2019

View reviewed changes

Leonardo2718 merged commit a77c9d5 into eclipse:master Nov 27, 2019

Leonardo2718 self-assigned this Nov 27, 2019

wbh123456 mentioned this pull request Nov 28, 2019

Remove references of deprecated const/compare Opcodes from OpenJ9 eclipse-openj9/openj9#7909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop generating deprecated unsigned opcodes of equality compare and const #4347

Stop generating deprecated unsigned opcodes of equality compare and const #4347

wbh123456 commented Sep 23, 2019

andrewcraik commented Sep 23, 2019

wbh123456 commented Sep 24, 2019

andrewcraik commented Sep 27, 2019

wbh123456 commented Sep 28, 2019

wbh123456 commented Oct 2, 2019

andrewcraik commented Oct 2, 2019

andrewcraik commented Oct 2, 2019

wbh123456 commented Oct 3, 2019

andrewcraik commented Oct 3, 2019

andrewcraik commented Oct 4, 2019

wbh123456 commented Oct 4, 2019

andrewcraik Oct 10, 2019

wbh123456 Oct 10, 2019

wbh123456 commented Oct 16, 2019

Leonardo2718 commented Oct 16, 2019

andrewcraik Oct 16, 2019

wbh123456 Oct 16, 2019

andrewcraik Oct 16, 2019

andrewcraik commented Oct 16, 2019

Leonardo2718 commented Oct 16, 2019

Leonardo2718 commented Oct 18, 2019

Leonardo2718 commented Oct 18, 2019

wbh123456 commented Oct 18, 2019

wbh123456 commented Oct 31, 2019 •

edited

Leonardo2718 commented Nov 11, 2019

Leonardo2718 commented Nov 27, 2019

andrewcraik commented Nov 27, 2019

wbh123456 commented Nov 27, 2019

andrewcraik commented Nov 27, 2019

andrewcraik commented Nov 27, 2019

Leonardo2718 left a comment

Stop generating deprecated unsigned opcodes of equality compare and const #4347

Stop generating deprecated unsigned opcodes of equality compare and const #4347

Conversation

wbh123456 commented Sep 23, 2019

andrewcraik commented Sep 23, 2019

wbh123456 commented Sep 24, 2019

andrewcraik commented Sep 27, 2019

wbh123456 commented Sep 28, 2019

wbh123456 commented Oct 2, 2019

andrewcraik commented Oct 2, 2019

andrewcraik commented Oct 2, 2019

wbh123456 commented Oct 3, 2019

andrewcraik commented Oct 3, 2019

andrewcraik commented Oct 4, 2019

wbh123456 commented Oct 4, 2019

andrewcraik Oct 10, 2019

Choose a reason for hiding this comment

wbh123456 Oct 10, 2019

Choose a reason for hiding this comment

wbh123456 commented Oct 16, 2019

Leonardo2718 commented Oct 16, 2019

andrewcraik Oct 16, 2019

Choose a reason for hiding this comment

wbh123456 Oct 16, 2019

Choose a reason for hiding this comment

andrewcraik Oct 16, 2019

Choose a reason for hiding this comment

andrewcraik commented Oct 16, 2019

Leonardo2718 commented Oct 16, 2019

Leonardo2718 commented Oct 18, 2019

Leonardo2718 commented Oct 18, 2019

wbh123456 commented Oct 18, 2019

wbh123456 commented Oct 31, 2019 • edited

Checks that have been done to ensure correctness and performance:

Leonardo2718 commented Nov 11, 2019

Leonardo2718 commented Nov 27, 2019

andrewcraik commented Nov 27, 2019

wbh123456 commented Nov 27, 2019

andrewcraik commented Nov 27, 2019

andrewcraik commented Nov 27, 2019

Leonardo2718 left a comment

Choose a reason for hiding this comment

wbh123456 commented Oct 31, 2019 •

edited