Enhance extensibility of JitBuilder classes

[mstoodle]

The JitBuilder implementation files in compiler/ilgen are not really in the right shape to be extended by other compilers. This pull request moves these files closer to the extensible classes shape used by some classes in the rest of the compiler, but not all the way (on purpose). I am willing to change my position on these aspects, but for now I want to see where we can get without them:

1. I strongly prefer not to introduce architectural variations in these files, so I only provided an OMR implementation without the usual Connector stuff that enables architectural specializations.
2. I also avoided the use of self() throughout these classes, which is an implicit statement that I am hoping extensions of these classes will only add function rather than override function.

Note for JitBuilder clients: OMR::VirtualMachine* classes have been promoted to full TR::VirtualMachine* classes. However, once the new client API work lands, all those classes are moving to the OMR::JitBuilder namespace anyway (sorry for the dual migration).

[mstoodle]

@genie-omr build all

[mstoodle]

@genie-omr build zlinux

[mstoodle]

@genie-omr build zlinux

[mstoodle]

(At least) One more time! @genie-omr build zlinux

[mstoodle]

@genie-omr build plinux

[mstoodle]

yay @genie-omr build all

[mstoodle]

For the record, I think this one is ready for your consideration when you're ready, @0xdaryl (which is to say: I think I'm finally done iterating through build failures!).

[mstoodle]

Latest push only fixed the merge conflicts induced by #2529 .

[0xdaryl]

With the constraints that you want, I wonder if we should start to use C++ language features to enforce some of these rules. Otherwise, your intentions will be lost over time. For instance, for the functions you expect to provide functionality for but do not want to be overridden, would you consider marking them virtual and use the C++11 final keyword to enable compile-time enforcement? Not every OMR build compiler supports the same level of C++11 unfortunately, but we could use an OMR_FINAL (for lack of a better name) macro in its place that maps to the final keyword on compiler versions that do support it and to nothing on those that don't. Very soon I suspect the OMR toolchain will be moving up to support newer compilers which means that at least some of the builds will be validating the proper use of the code as you intended (which is better than nothing), and hopefully armed with better devirtualization optimizations for final functions.

On a related note, there is an experiment underway to "revirtualize" the extension points in the extensible class hierarchy. That is, turn the extensibility points of a class into virtual functions and thereby eliminate the requirement for the self() functions. Using virtual functions as extension points is also useful in that we can introduce pure virtual functions for those extension points that must be supplied by a consuming class (for which there isn't a sensible default OMR implementation). I expect an issue to be created to describe this work soon, and it will be a discussion topic at the June 6 compiler architecture meeting.

[charliegracie]

I would really recommend against the use of C++11 features. I am actively trying to get all current uses of C++11 features removed from the codebase as they are not supported on all of our platforms / compilers.

[mstoodle]

@0xdaryl I'm not sure I understand your suggestion. What is the benefit to make a function virtual but marked as final versus just not making it virtual in the first place?

[mstoodle]

oh, i see, to prevent a subclass from overriding it anyway which would be visible via calls to the TR namespaced class but not to calls within that class's hierarchy (i.e. calls implicitly or explicitly via this)

[mstoodle]

having all methods be tagged with virtual OMR_FINAL feels pretty ugly though :( .

[charliegracie]

I would agree that tagging all of the functions would look ugly and not really add any value. @mstoodle can you rebase to get rid of the merge conflicts? @0xdaryl is there anything else you would like to see changed?

[mstoodle]

@genie-omr build all

[mstoodle]

@charliegracie @0xdaryl rebased now. Anything else you think should be changed at this point?

[0xdaryl]

I don't believe we should preclude the use of more modern C++ features if it makes understanding the code easier or allows the compiler to perform compile-time checks to enforce rules we have in the code base. Even more so if it avoids the development of specialized "linter" tools to catch transgressions. This is especially true if that C++ feature can easily be avoided on build compilers that do not support said feature (for instance, by simply hiding its use behind a macro). I get the argument that we need to avoid "modern" language features in order to increase broadest possible adoption, but there is also an argument to be made that some of those features are actually useful and beneficial to some projects. Precluding new language features may in fact be punishing those projects that choose to build with more modern toolchains by leaving important performance optimizations on the table or limiting their effect. It's a bit of a double-edged sword.

This PR is not the right forum to debate this topic and likely needs a broader project-level discussion to define the minimum language level that will be accepted and the minimum compiler levels (and there should be CI jobs that test them). We don't have a clearly documented standard to apply to changes proposed in a PR as to whether they're acceptable from a toolchain perspective or not. And since we don't, debates such as this will recur. Perhaps this should be part of the OMR release discussion (#2274) or make the toolchain part of the API we define for a release.

In the interests of moving this PR forward, however, I don't think there's any particular technique employed in this PR that's outside the spirit of extensible classes.

@mstoodle, the PR does need a rebase though.

[mstoodle]

I'll rebase tonight and ping when it's ready. Thanks Daryl!

[mstoodle]

For extra insurance: @genie-omr build all