Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize evaluator names across backends #5720

Merged
merged 11 commits into from
Jun 1, 2021

Conversation

fjeremic
Copy link
Contributor

@fjeremic fjeremic commented Dec 11, 2020

Standardize evaluator names across all backends to be of the form:

<ilName>Evaluator

The tree evaluator table is now generated using Opcodes.enum IL macro definitions and has been commoned up in the common code generator. Backends must implement all of the evaluator functions to be able to compile. As we continue our unification journey we can start to pull up common definitions into the common codegen. For example we can reach a state where all the evaluators are implemented in the common codegen to delegate to unImpOpEvaluator as an example.

Further, backends and downstream projects are now able to override evaluator implementations by using the extensible class mechanism and simply defining a function in the TreeEvaluator class. For example in a hypothetical downstream project, one can do:

DownstreamProject::X86::TreeEvaluator::iaddEvaluator(TR::Node *node, TR::CodeGenerator *cg)
{
    // My custom iadd evaluator
}

Closes: #5703

fjeremic added 4 commits May 26, 2021 13:08
Now that we have an evaluator for each and every IL opcode, we can no
longer compare against `unImpOpEvaluator` or `badILOpEvaluator` to
determine whether we have implemented an evaluator for that specific IL.

Instead we have to override this function and manually return false
for the specific set of unimplemented opcodes.
See previous commit for explanation. Same work was performed on x86.
@fjeremic
Copy link
Contributor Author

Jenkins build all

@fjeremic fjeremic changed the title WIP: Canonicalize the tree evaluators Standardize evaluator names across backends May 26, 2021
@fjeremic
Copy link
Contributor Author

@dsouzai / @0xdaryl this will require a coordinated merge from one of you.

Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the two Override isILOpCodeSupported commits. Because of

bool OMR::CodeGenerator::isILOpCodeSupported(TR::ILOpCodes o)
   {
	return (_nodeToInstrEvaluators[o] != TR::TreeEvaluator::unImpOpEvaluator) &&
	      (_nodeToInstrEvaluators[o] != TR::TreeEvaluator::badILOpEvaluator);
   }

given that all opcodes have their own evaluator now, OMR::CodeGenerator::isILOpCodeSupported is never going to return true. So why even bother having a case for TR::a2i? Why not just implement the evaluator for it and have it call TR::TreeEvaluator::unImpOpEvaluator?

But if the old behaviour is desired, then shouldn't there be way more cases for all of the different evaluators that are basically a wrapper around TR::TreeEvaluator::unImpOpEvaluator/TR::TreeEvaluator::badILOpEvaluator and shouldn't these be overriden in all platforms?

@fjeremic
Copy link
Contributor Author

fjeremic commented May 31, 2021

But if the old behaviour is desired, then shouldn't there be way more cases for all of the different evaluators that are basically a wrapper around TR::TreeEvaluator::unImpOpEvaluator/TR::TreeEvaluator::badILOpEvaluator and shouldn't these be overriden in all platforms?

There should be, but there appears to be currently no tests for such IL. The only such case I encountered was for a2i which has tests in the compilertest directory (non-Tril unit tests). The current solution is not great because isILOpCodeSupported is incomplete. However rather than going through each codegen and figuring out the set of IL that it doesn't support and adding it to the various lists there, we instead did the bare minimum to get all the tests to pass.

If someone introduces additional tests for IL that is not supported on some platforms, the tests will fail and we'll need to update isILOpCodeSupported at that time. Honestly though, I think this API should not exist at all. Our IL claims to be platform agnostic, and it should be implemented as such.

@dsouzai
Copy link
Contributor

dsouzai commented Jun 1, 2021

Alright, I can accept that as a current limitation. Probably worth opening an issue we can link to the housekeeping efforts (unless it's already being addressed by you in a future PR).

@dsouzai dsouzai merged commit 67a5f4f into eclipse-omr:master Jun 1, 2021
@knn-k
Copy link
Contributor

knn-k commented Jun 2, 2021

I opened #6040 to remove duplicated definitions of some evaluators for AArch64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up centralized opcode enums
3 participants