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

Discussion: Add ability to have compiler option bits in OpenJ9 #20314

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dsouzai
Copy link
Contributor

@dsouzai dsouzai commented Oct 7, 2024

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, #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.

@jdmpapin
Copy link
Contributor

jdmpapin commented Oct 7, 2024

This approach seems like an improvement to me. A few notes:

  • I hope the 64-bit enum doesn't pose a problem for any of our build compilers
  • I think RE/SET_OPTION_BIT should probably shift down before casting to uintptr_t instead of afterward. The cast will truncate the upper half on 32-bit
  • (1<<32) should probably cast the 1 to uint64_t before shifting to avoid overflow shifting an int

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.

2 participants