Refactoring of TR::ternary opcodes to TR::select #4578

alvee-unb · 2019-11-20T14:04:28Z

Refactoring of TR::ternary opcodes to TR::select

Replacing of the ternary OpCodes such as TR::bternary etc. to
TR::bselect etc. and the associated items respectively.

The changes are made onto the followings:

Components of various data types such as- Int8, Float, Double
(iternary, bternary etc) etc..
Corresponding evaluators such as- bselectEvaluator, iselectEvaluator
etc. and other functions where the IL OpCodes are used.
Corresponding outputs of texts such as- "iselect" etc. and comments
(//ternary).

Closes: GitHub issue #681

Signed-off-by: Md. Alvee Noor mnoor@unb.ca

andrewcraik · 2019-11-20T14:44:54Z

Shouldn't the opCodeForTernarySelect also be renamed as part of this?

0xdaryl

Thanks for your first contribution! I did an initial pass through this and found a few things that need to get fixed up. Once they are, on my next pass I will checkout your branch and sniff around the code for other "ternaries" that may have been missed (other than the one that @andrewcraik already mentioned).

0xdaryl · 2019-11-22T02:42:26Z

compiler/il/OMRIL.cpp

-   TR::fternary, // Float
-   TR::dternary, // Double
-   TR::aternary, // Address
+   TR::bselect, // Int8. Formerly- TR::bternary


Why are these ternary comments necessary? Please remove.

0xdaryl · 2019-11-22T02:43:43Z

compiler/il/OMRILOpCodesEnum.hpp

-   aternary,   //
-   fternary,   //
-   dternary,   //
+   iselect,   // Former Ternary Operator:  Based on the result of the first child, take the value of the


Replace "Former Ternary Operator" with "Select Operator"

0xdaryl · 2019-11-22T02:44:13Z

compiler/il/OMRILOpCodesEnum.hpp

-   dternary,   //
+   iselect,   // Former Ternary Operator:  Based on the result of the first child, take the value of the
+               //  second (first child evaluates to true) or third(first child evaluates to false) child
+               //  Formerly iternary, lternary, bternary, sternary etc..


Remove the "Formerly..." comment.

0xdaryl · 2019-11-22T02:46:40Z

compiler/il/OMRILOpCodesEnum.hpp

@@ -574,7 +576,7 @@
   vreturn,    // return a vector
   vcall,      // direct call returning a vector
   vcalli,     // indirect call returning a vector
-   vternary,   // vector ternary operator
+   vternary,    // vector ternary operator


Rename to vselect as well.

0xdaryl · 2019-11-22T02:47:25Z

compiler/ilgen/OMRIlBuilder.hpp

@@ -586,7 +586,7 @@ class IlBuilder : public TR::IlInjector
                     TR::IlBuilder **caseBuilder,
                     int32_t caseFallsThrough);

-   // ternary
+   // select (former ternary)


Remove former ternary

0xdaryl · 2019-11-22T03:25:08Z

compiler/z/codegen/OMRTreeEvaluator.cpp

@@ -14424,7 +14424,7 @@ OMR::Z::TreeEvaluator::inlineVectorBinaryOp(TR::Node * node, TR::CodeGenerator *
   }

 TR::Register *
-OMR::Z::TreeEvaluator::inlineVectorTernaryOp(TR::Node * node, TR::CodeGenerator *cg, TR::InstOpCode::Mnemonic op)
+OMR::Z::TreeEvaluator::inlineVectorSelectOp(TR::Node * node, TR::CodeGenerator *cg, TR::InstOpCode::Mnemonic op)
   {
   TR_ASSERT(node->getNumChildren() >= 3,"Ternary Node must contain 3 or more children");


"Select node"

0xdaryl · 2019-11-22T03:25:31Z

compiler/z/codegen/OMRTreeEvaluatorTable.hpp

@@ -546,7 +546,7 @@
   TR::TreeEvaluator::vreturnEvaluator,     // TR::vreturn
   TR::TreeEvaluator::directCallEvaluator,  // TR::vcall
   TR::TreeEvaluator::indirectCallEvaluator,// TR::vcalli
-   TR::TreeEvaluator::ternaryEvaluator,     // TR::vternary
+   TR::TreeEvaluator::selectEvaluator,      // TR::vternary


0xdaryl · 2019-11-22T03:26:22Z

fvtest/compilertest/tests/OpCodesTest.hpp

-   static signatureCharIJJ_J_testMethodType *_lternary;
-   static signatureCharIFF_F_testMethodType *_fternary;
-   static signatureCharIDD_D_testMethodType *_dternary;
+   //Select (former- Ternary) operators


Remove former ternary

0xdaryl · 2019-11-22T03:26:46Z

fvtest/compilertest/tests/injectors/OpIlInjector.hpp

@@ -133,7 +133,7 @@ class OpIlInjector : public TR::IlInjector

   TR::ILOpCodes _opCode;
   TR::DataType _dataType; // datatype of OpCode child/children
-   TR::DataType _conditionalDataType; // return type of Ternary opcodes
+   TR::DataType _conditionalDataType; // return type of Select (former- Ternary) opcodes


Remove former ternary

0xdaryl · 2019-11-22T03:27:01Z

fvtest/compilertriltest/TernaryTest.cpp

 *
 *  \tparam CompareType
- *     The type of the compare child of the ternary opcode.
+ *     The type of the compare child of the select (former- ternary) opcode.


Remove former ternary

0xdaryl · 2019-11-26T16:55:27Z

compiler/il/OMRILOpCodesEnum.hpp

@@ -500,7 +500,7 @@
   vicmpanyle,       // vector integer any less equal

   vnot,          // vector boolean not
-   vselect,       // vector select
+   vbitselect,       // vector select


Perhaps // vector bit select

0xdaryl · 2019-11-26T17:35:01Z

compiler/p/codegen/OMRTreeEvaluator.cpp

   {
-   return TR::TreeEvaluator::inlineVectorTernaryOp(node, cg, TR::InstOpCode::vsel);
+   return TR::TreeEvaluator::inlineVectorSelectOp(node, cg, TR::InstOpCode::vsel);


I believe this function should be inlineVectorBitSelectOp since the opcode was renamed. This comment applies elsewhere to the other evaluators that call it too.

0xdaryl · 2019-11-26T17:40:39Z

compiler/z/codegen/OMRTreeEvaluatorTable.hpp

@@ -478,7 +478,7 @@
   TR::TreeEvaluator::unImpOpEvaluator,         //  TR::vicmpanylt
   TR::TreeEvaluator::unImpOpEvaluator,         //  TR::vicmpanyle
   TR::TreeEvaluator::unImpOpEvaluator,         //  TR::vnot
-   TR::TreeEvaluator::vselEvaluator,        //  TR::vselect
+   TR::TreeEvaluator::vselEvaluator,        //  TR::vbitselect


This should now be TR::TreeEvaluator::unImpOpEvaluator as discussed in #4590.

I believe the issue #4590 refers to the new vselect opcode to be unimplemented in the Z tree evaluator table (which is done), not the vbitselect. Can you please re-confirm? Thanks.

Yes, sorry.

0xdaryl · 2019-11-26T18:20:02Z

Looking at what is left after this commit is applied there are a few things that stand out:

Rename rematerializeCmpUnderTernary
Rename:

il/OMRILProps.hpp:      Ternary                      = 0x00008000
il/OMRILOps.hpp:   bool isTernary()                  const { return properties2().testAny(ILProp2::Ternary); }

Rename:

il/OMRIL.hpp:   static TR::ILOpCodes opCodesForTernarySelect[];
il/OMRIL.hpp:   TR::ILOpCodes opCodeForTernarySelect(TR::DataType dt);

There are a number of comments that reference "ternary nodes" and such. These should probably be renamed to "select"

You have to be careful though because some of these have uses in downstream projects like OpenJ9. What I suggest you do is to eliminate downstream breakages as much as possible when renaming. For example, you can duplicate the isTernary function (as isSelect), when that change lands go and fix up the uses in OpenJ9, and then return to OMR and clean things up.

0xdaryl · 2019-12-18T14:52:50Z

Be aware of #4681 as it is adding a new flag for "ternary" opcodes. Some of that new code will have to change if that PR lands first.

alvee-unb · 2019-12-20T20:01:27Z

Be aware of #4681 as it is adding a new flag for "ternary" opcodes. Some of that new code will have to change if that PR lands first.

Hello @0xdaryl , as that PR landed first, I have updated the code accordingly and force-pushed again. Would appreciate a review, thank you.

0xdaryl · 2020-01-15T14:12:59Z

@genie-omr build all

0xdaryl

The bulk of these changes look good to me. I just have a few minor points in the comments.

eclipse-openj9/openj9#8364 has introduced a new use of aternary in OpenJ9. In order for this PR in OMR to go in smoothly first, I suggest leaving the following in OMR for now:

keep aternary in the enum that shares the same enum value as aselect
keep the CodeGenerator functions getSupportsTernary()

These can be cleaned up (along with removing isTernaryHigh) once your corresponding OpenJ9 PR lands.

0xdaryl · 2020-01-29T13:12:24Z

compiler/il/OMRIL.cpp

+TR::ILOpCodes
+OMR::IL::opCodeForSelect(TR::DataType dt)
+   {
+   TR_ASSERT(dt < TR::NumOMRTypes, "unexpcted opcode");


Should be "Unexpected data type". Also fix spelling of 'unexpected'.

0xdaryl · 2020-01-29T13:18:16Z

compiler/il/OMRILOps.hpp

+   bool isSelect()                  const { return properties2().testAny(ILProp2::Select); }
+   bool isSelectAdd()               const { return properties2().testAny(ILProp2::SelectAdd); }
+   bool isSelectSub()               const { return properties2().testAny(ILProp2::SelectSub); }


Please add a single space before const to maintain alignment.

0xdaryl · 2020-01-29T13:19:20Z

compiler/il/OMRILProps.hpp

+      SelectAdd                   = 0x00010000,
+      SelectSub                   = 0x00020000,


Please add a space before = to maintain alignment.

0xdaryl · 2020-01-29T13:31:05Z

compiler/il/OMRNode.cpp

@@ -2084,6 +2084,25 @@ OMR::Node::isTernaryHigh()
   return false;
   }

+bool
+OMR::Node::isSelectHigh()
+   {


If isTernaryHigh() is just a stub now (temporarily), I would rather it's body be gutted and simply return self()->isSelectHigh(); rather than identically duplicate this body.

0xdaryl · 2020-01-29T13:44:26Z

compiler/riscv/codegen/OMRTreeEvaluator.hpp

+	static TR::Register *iuselectEvaluator(TR::Node *node, TR::CodeGenerator *cg);
+	static TR::Register *luselectEvaluator(TR::Node *node, TR::CodeGenerator *cg);
+	static TR::Register *buselectEvaluator(TR::Node *node, TR::CodeGenerator *cg);
+	static TR::Register *suselectEvaluator(TR::Node *node, TR::CodeGenerator *cg);


I think these can just be deleted. There are no references and the opcode doesn't exist.

alvee-unb · 2020-02-19T14:41:54Z

Hi @0xdaryl , @knn-k , I would appreciate a review on my most recent commit. Thank you.

0xdaryl · 2020-02-19T17:59:24Z

Thanks for the changes. Please don't sprinkle the code with comment references to the OpenJ9 PR eclipse-openj9/openj9#7797. While it is true there is cleanup needed after that is merged, I'd rather you open an issue in OMR to track this removal (and all the other things you need to remove once downstream changes merge) than leave a comments in the code. I saw several comments in the code that referred to that downstream issue.

0xdaryl · 2020-02-19T20:08:22Z

@genie-omr build all

andrewcraik · 2020-02-24T15:12:56Z

compiler/il/OMRNode.hpp

@@ -533,6 +533,7 @@ class OMR_EXTENSIBLE Node
   /// and the opcodes for highOp/adjunctOp are luaddc/luadd, or lusubb/lusub.
   ///
   bool                   isTernaryHigh();
+   bool                   isSelectHigh();


The doxygen should be on isSelectHigh with a deprecated warning on isTernaryHigh.

Thank you for your comment, I am updating this for other similar instances (on compiler/il/OMRNode.cpp).

Replacing of the ternary OpCodes such as TR::bternary etc. to TR::bselect etc. and the associated items respectively. The changes are made onto the followings: - Components of various data types such as- Int8, Float, Double (iternary, bternary, vternary etc) etc.. - Corresponding evaluators such as- bselectEvaluator, iselectEvaluator etc. and other functions where the IL OpCodes are used. - Corresponding outputs of texts such as- "iselect" etc. and comments (//ternary). - The IL opcode (and its respective evaluator to- vbitselectEvaluator)- vselect has been updated to vbitselect. - The new vselect opcode has been marked- unimplemented in the Z tree evaluator. - File names which involved updating of the class names. - There are some duplicated usages of the opcodes (`aternary`) and their respective functions for their dependencies on OpenJ9, they will need to be deleted once the eclipse/openj9 #7797 PR is accepted. Partially Resolves: GitHub issues eclipse#681 and eclipse#4590 Signed-off-by: Md. Alvee Noor <mnoor@unb.ca>

Removed deprecated functions (i.e. `getSupportsTernary` etc.) which were introduced by the PR eclipse#4578. Closes: GitHub issue eclipse#681 Signed-off-by: Md. Alvee Noor <mnoor@unb.ca>

Removed deprecated functions (i.e. `getSupportsTernary` etc.) which were introduced by the PR eclipse#4578. Closes: GitHub issue eclipse#681 and eclipse#4847 Signed-off-by: Md. Alvee Noor <mnoor@unb.ca>

0xdaryl · 2020-02-26T13:38:11Z

@genie-omr build all

0xdaryl · 2020-02-26T17:26:28Z

macOS failure is a late infrastructure failure after the tests have succeeded. Merging.

Replaced the ternary OpCodes such as TR::aternary etc. to TR::aselect etc. respectively. Related OMR PR: eclipse/omr#4578 Partially Resolves: Issue eclipse/omr#681 and eclipse/omr#4590 Signed-off-by: Md. Alvee Noor <mnoor@unb.ca>

Removed deprecated functions (i.e. `getSupportsTernary` etc.) which were introduced by the PR eclipse#4578. Closes: GitHub issue eclipse#681 and eclipse#4847 Signed-off-by: Md. Alvee Noor <mnoor@unb.ca>

alvee-unb requested review from 0xdaryl, andrewcraik, charliegracie, fjeremic, Leonardo2718, mstoodle and vijaysun-omr as code owners November 20, 2019 14:04

alvee-unb mentioned this pull request Nov 20, 2019

Refactoring of TR::ternary opcodes to TR::select eclipse-openj9/openj9#7797

Merged

Leonardo2718 added the comp:compiler label Nov 20, 2019

0xdaryl requested changes Nov 22, 2019

View reviewed changes

aarongraham9 mentioned this pull request Nov 22, 2019

vternary and vselect opcodes #4590

Closed

alvee-unb force-pushed the Refactor-TernaryToSelect branch 2 times, most recently from c90236d to 9ca7659 Compare November 25, 2019 15:30

0xdaryl reviewed Nov 26, 2019

View reviewed changes

alvee-unb changed the title ~~Refactoring of TR::ternary opcodes to TR::select~~ WIP: Refactoring of TR::ternary opcodes to TR::select Nov 27, 2019

alvee-unb force-pushed the Refactor-TernaryToSelect branch from 9ca7659 to e8f48d1 Compare December 16, 2019 18:59

alvee-unb requested review from rwy7 and youngar as code owners December 16, 2019 18:59

alvee-unb changed the title ~~WIP: Refactoring of TR::ternary opcodes to TR::select~~ Refactoring of TR::ternary opcodes to TR::select Dec 16, 2019

alvee-unb force-pushed the Refactor-TernaryToSelect branch from e8f48d1 to 3948cd5 Compare December 16, 2019 21:36

alvee-unb requested a review from 0xdaryl December 17, 2019 19:33

0xdaryl mentioned this pull request Dec 18, 2019

Enable simplifying conditional store in CFG simplifier on x, p,… #4681

Merged

alvee-unb force-pushed the Refactor-TernaryToSelect branch from 3948cd5 to 097199d Compare December 20, 2019 19:40

alvee-unb force-pushed the Refactor-TernaryToSelect branch from 097199d to 4d02352 Compare January 14, 2020 13:46

alvee-unb force-pushed the Refactor-TernaryToSelect branch from 4d02352 to 1ad446f Compare January 14, 2020 18:22

hzongaro mentioned this pull request Jan 23, 2020

Avoid control flow for copying heap allocations to temporaries eclipse-openj9/openj9#8364

Merged

0xdaryl requested changes Jan 29, 2020

View reviewed changes

alvee-unb force-pushed the Refactor-TernaryToSelect branch from 1ad446f to 62e6925 Compare February 12, 2020 21:52

alvee-unb mentioned this pull request Feb 19, 2020

Cleanup duplicated code created as part of the PR eclipse/omr#4578 #4847

Closed

alvee-unb force-pushed the Refactor-TernaryToSelect branch from 62e6925 to e10c9e0 Compare February 19, 2020 18:59

0xdaryl self-assigned this Feb 20, 2020

andrewcraik reviewed Feb 24, 2020

View reviewed changes

alvee-unb force-pushed the Refactor-TernaryToSelect branch from e10c9e0 to 66aae96 Compare February 25, 2020 20:54

alvee-unb mentioned this pull request Feb 25, 2020

Refactor ternary to select cleanup #4865

Merged

0xdaryl approved these changes Feb 26, 2020

View reviewed changes

0xdaryl merged commit a67e389 into eclipse:master Feb 26, 2020

aarongraham9 deleted the Refactor-TernaryToSelect branch February 16, 2021 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of TR::ternary opcodes to TR::select #4578

Refactoring of TR::ternary opcodes to TR::select #4578

alvee-unb commented Nov 20, 2019

andrewcraik commented Nov 20, 2019

0xdaryl left a comment

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 22, 2019

0xdaryl Nov 26, 2019

0xdaryl Nov 26, 2019

0xdaryl Nov 26, 2019

alvee-unb Nov 28, 2019

0xdaryl Nov 28, 2019

0xdaryl commented Nov 26, 2019

0xdaryl commented Dec 18, 2019

alvee-unb commented Dec 20, 2019

0xdaryl commented Jan 15, 2020

0xdaryl left a comment

0xdaryl Jan 29, 2020

0xdaryl Jan 29, 2020

0xdaryl Jan 29, 2020

0xdaryl Jan 29, 2020

0xdaryl Jan 29, 2020

alvee-unb commented Feb 19, 2020

0xdaryl commented Feb 19, 2020

0xdaryl commented Feb 19, 2020

andrewcraik Feb 24, 2020

alvee-unb Feb 25, 2020 •

edited

Loading

0xdaryl commented Feb 26, 2020

0xdaryl commented Feb 26, 2020

Refactoring of TR::ternary opcodes to TR::select #4578

Refactoring of TR::ternary opcodes to TR::select #4578

Conversation

alvee-unb commented Nov 20, 2019

andrewcraik commented Nov 20, 2019

0xdaryl left a 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

0xdaryl commented Nov 26, 2019

0xdaryl commented Dec 18, 2019

alvee-unb commented Dec 20, 2019

0xdaryl commented Jan 15, 2020

0xdaryl left a 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

alvee-unb commented Feb 19, 2020

0xdaryl commented Feb 19, 2020

0xdaryl commented Feb 19, 2020

Choose a reason for hiding this comment

alvee-unb Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

0xdaryl commented Feb 26, 2020

0xdaryl commented Feb 26, 2020

alvee-unb Feb 25, 2020 •

edited

Loading