openj9 icon indicating copy to clipboard operation
openj9 copied to clipboard

Discussion: Add ability to have compiler option bits in OpenJ9

Open dsouzai opened this issue 1 year ago • 1 comments

Opening this draft PR for discussion purposes.

Motivation

Whenever we need to guard exclusively OpenJ9 compiler code with an -Xjit/-Xaot option bit, we have to open a PR in the Eclipse OMR repo. This is both tedious (as it involves 2 PRs) and adds unnecessary noise to the Eclipse OMR project. I decided to try and prototype option bits in OpenJ9, and it's actually fairly straightforward to implement.

Implementation Details

This draft PR demonstrates how to do so. It only needs the following change in OMR:

diff --git a/compiler/control/OMROptions.cpp b/compiler/control/OMROptions.cpp
index f44e0b48de..4a73b4ea18 100644
--- a/compiler/control/OMROptions.cpp
+++ b/compiler/control/OMROptions.cpp
@@ -3542,7 +3542,7 @@ OMR::Options::processOptionSet(
             return options;
             }

-         const char *feEndOpt = TR::Options::processOption(options, TR::Options::_feOptions, _feBase, _numVmEntries, optionSet);
+         const char *feEndOpt = TR::Options::processOption(options, TR::Options::_feOptions, jitBase, _numVmEntries, optionSet);

          if (!feEndOpt)
             {

However, this change in OMR is, IMO, not only OK to implement, but also functionally necessary as well as strengthens conceptual integrity. _feBase is the JITConfig (and not the TR_FrontEnd/TR_J9VMBase), but furthermore, SET_OPTION_BIT/RESET_OPTION_BIT needs the processOptionSet param jitBase to be a TR::Options object; in fact to set bits in the JITConfig, there exists the set32BitNumericInJitConfig method. For these reasons, even though _feOptions is passed to processOption, the base must be the TR::Options object, and not anything else, both for functional and conceptual integrity reasons.

Criticism

This implementation doesn't fully conform to the extensible classes philosophy, since _j9options isn't an extension of _options

My main purpose was to demonstrate that option bits can exist in OpenJ9; the implementation in this PR is just one way of ensuring overlapping enums literals are not possible.

This is just a stop gap; a better solution should be implemented

While this is a fair point, I'm also being pragmatic about distinguishing what should happen and what can happen. For example, https://github.com/eclipse-openj9/openj9/issues/16289 proposed enabling merging of -Xjit options in JDK25 (as per the comments), but because that seemed to so far out at the time, there was hope that we'd have sorted out how -Xjit options should be by JDK25. However, that is unlikely to happen unless someone champions the effort and completes it in the next 9 months.

So, if something simple like this that is a relative small amount of work that

  1. improves developer experience
  2. does not hinder significantly a proper potential future Options solution

then I don't think something like this should be discouraged.

dsouzai avatar Oct 07 '24 19:10 dsouzai