Rename disableTOCForConsts option and disable TOC when the opti…

[dchopra001]

This PR renames the disableTOCForConstants option to just disableTOC, and then uses the option to prevent the PPC code generator from being able to generate TOC code.

Signed-off-by: Dhruv Chopra Dhruv.C.Chopra@ibm.com

[dchopra001]

I'm not sure if there's a better way to disable the TOC (a more central location). I tried to follow the example of how it is done in AOT. Please let me know if there's a better way to do this.

Also, we will probably need a different query since isOutOfProcessCompilation() won't work in OMR.

[dchopra001]

This PR is part of the work in eclipse-openj9/openj9#6991

[dchopra001]

@ymanton For this PR we can't use the isOutOfProcessCompilation routine. So as an alternative I can add a codegen level query like below:

cg->isTOCEnabledForCompilesWithRelocatableInstructions()

This query will return true in OMR, but will return !cg->comp()->isOutOfProcessCompilation() when overriden in OpenJ9. This would be a simple and least invasive change. Many of the routines modified inside this PR already have similar code (ex. if (compileRelocatableCode) return PTOC_FULL_INDEX;). So this change would fit in.

Another alternative is something you suggested in another PR where we would make TR_PPCTableOfConstants an extensible class. Then I suppose we can add a query inside there to check whether TOC is enabled or not using a similar check to the one above. At the OMR level, I think the code inside these TOC methods will look similar. But this second approach will avoid introducing another query in the codegen and will keep the query confined to the TOC class. It can also be a longer term solution (we can throw in the other checks like if (compileRelocatableCode) that are in many of these routines already) where all checks can go through a common point. However this may take longer as more testing will be needed to ensure no corner cases are missed.

Both of these approaches will also allow us to toggle this behaviour easily when we eventually enable TOC in JITServer mode. Additionally, there aren't much more places around the codebase where we will need to use these queries, as most of the TOC related code goes through one of pre-existing routines modified in this PR.

Let me know which one you think is better or if there's a better alternative.

[ymanton]

Here's another alternative that doesn't require making the TOC extensible: We have an option called disableTOCForConsts, can we re-use that to reduce the number of checks we do and allow downstream projects to disable the TOC as well?

• First, rename it to disableTOC and update the help text because the "ForConsts" doesn't add anything.
• Then in the lookUp() functions we can check just for that option to decide whether to return PTOC_FULL_INDEX or not.
• In OpenJ9's options processing we would then have to do the various checks that we do now in lookUp() and set the disableTOC option when necessary.
• We would also have to make sure that the parts of the code that currently check disableTOC still make sense and fix them if neccessary. I think the ARM codegen looks at that option as well.

[dchopra001]

We would also have to make sure that the parts of the code that currently check disableTOC still make sense and fix them if neccessary. I think the ARM codegen looks at that option as well.

In the ARM codegen we only use this option inside here:
https://github.com/eclipse/omr/blob/d2e17b187b33aabcfbbd0d94026f93f78296a088/compiler/arm/codegen/ConstantDataSnippet.cpp#L76-L105

And this code is guarded by an #if 0, so in the immediate future I don't think changing the name of the option here will make a difference. Additionally, the Power codegen uses this option in a very similar way:
https://github.com/eclipse/omr/blob/d2e17b187b33aabcfbbd0d94026f93f78296a088/compiler/p/codegen/OMRConstantDataSnippet.cpp#L84-L113

So I think it should be safe renaming and the uses of the constants looks okay.

The only other place in the power codegen where this option is used is:
https://github.com/eclipse/omr/blob/d2e17b187b33aabcfbbd0d94026f93f78296a088/compiler/p/codegen/OMRTreeEvaluator.cpp#L220-L221

And given the context, I think this is reasonable as well.

[dchopra001]

@ymanton As discussed offline, I've renamed the option here, and made the appropriate changes in the codegen to prevent TOC code from being generated when the option is set.

[dchopra001]

See eclipse-openj9/openj9#7958 for related OpenJ9 PR. The TOC will now be disabled in the downstream project in JITServer mode only.

[ymanton]

Most of it looks fine to me.

[dchopra001]

@ymanton Ready for another review.

[ymanton]

LGTM.

[fjeremic]

@genie-omr build all

[dchopra001]

x86 build failed with:

14:41:11  CMake Error at tools/hookgen/CMakeLists.txt:24 (add_executable):
14:41:11    The install of the hookgen target requires changing an RPATH from the build
14:41:11    tree, but this is not supported with the Ninja generator unless on an
14:41:11    ELF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH variable may be set
14:41:11    to avoid this relinking step.
14:41:11  
14:41:11  
14:41:11  CMake Error at tools/tracemerge/CMakeLists.txt:23 (add_executable):
14:41:11    The install of the tracemerge target requires changing an RPATH from the
14:41:11    build tree, but this is not supported with the Ninja generator unless on an
14:41:11    ELF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH variable may be set
14:41:11    to avoid this relinking step.
14:41:11  
14:41:11  
14:41:11  CMake Error at tools/tracegen/CMakeLists.txt:23 (add_executable):
14:41:11    The install of the tracegen target requires changing an RPATH from the
14:41:11    build tree, but this is not supported with the Ninja generator unless on an
14:41:11    ELF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH variable may be set
14:41:11    to avoid this relinking step.
14:41:11  
14:41:11  
14:41:11  CMake Error at omrsigcompat/CMakeLists.txt:25 (add_library):
14:41:11    The install of the omrsig target requires changing an RPATH from the build
14:41:11    tree, but this is not supported with the Ninja generator unless on an
14:41:11    ELF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH variable may be set
14:41:11    to avoid this relinking step.

ARM build failed with:

ERROR: missing workspace /home/jenkins/workspace/PullRequest-linux_arm on ub1604-x86-unb-02
Finished: FAILURE

These don't look related to this PR.

[fjeremic]

Yeah I just checked the ARM change in the PR is just a rename and the Power has actual code changes which passed. I feel comfortable merging this in light of the infrastructure failures above.