Add isRelocatable boolean to power's loadAddressConstant API

[dchopra001]

This PR overloads the loadAddressConstant API on Power. We add a new boolean which allows the consumer of this API to determine if relocatable code needs to be generated.

Once the commits are merged and have propagated into downstream projects, we will make the necessary API changes there and remove the old definition of the API since it will no longer be needed.

[dchopra001]

More uses of this new API can be seen in #4442

[dchopra001]

I have separated this PR into 3 commits to provide some logical flow and to make it easier to review.
27d55ea implements the API change, while the remaining two commits make use of the new API. The commit messages also provide some context into why the changes for each particular case was needed.

A note on the API change:
The problem this API change is trying to solve is that loadAddressConstant won't always generate relocatable code because compileRelocatableCode won't always return the correct answer when doing a remote compilation.

Adding a new parameter to the function definition was a quick and flexible decision at the time that I was working on implementing JITServer support on Power. I'm not sure if this is the best long term solution for this problem. This is why I've opened this PR as a draft, so that we can decide on the final solution here.

A better solution might be to replace the if(cg->compileRelocatableCode()) check inside loadAddressConstant with a more general query that will always return the right result (whether doing a remote compilation or not). We will have to keep in mind other uses of this API that have caused issues so that we can capture all scenarios with the new query (available in #4442).

Also, isOutOfProcessCompilation() won't work in OMR, so we definitely can't use that here.

[dchopra001]

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

[dchopra001]

@ymanton Any thoughts on this API change? I can consolidate the if (cg->comp()->compileRelocatableCode() || isRelocatable) into a simple if (isRelocatable) change as well. But this will be a more invasive change as all callers will need to be modified as well.

Or please let me know if there's a better alternative.

[ymanton]

I can consolidate the if (cg->comp()->compileRelocatableCode() || isRelocatable) into a simple if (isRelocatable) change as well. But this will be a more invasive change as all callers will need to be modified as well.

I think that would be preferable because it makes things simpler inside loadAddressConstant(). If someone reads this code now they'll wonder why this routine has two different ways to check for the same thing. Just remove the default = false, see what breaks, and you should be able to fix most of the uses by just calling compileRelocatableCode(). If it makes things more complicated then we can revert back to this.

[ymanton]

I also looked at how we handle the table IL op which is similar, and that one is missing a JITServer check too, except it's not obvious. The data for that one is actually a table of branch instructions that are generated in the method itself, but we need to load the address of the table into a register and that needs relocation. The way we get that address into a register is by generating a label prior to the table and then calling fixedLoadLabelAddressIntoReg() which will call addMetaDataFor64BitFixedLoadLabelAddressIntoReg() which will check compileRelocatableCode().

Since addMetaDataFor64BitFixedLoadLabelAddressIntoReg() is extensible we can just extend it in OpenJ9 and add a JITServer check, but we can also either use the query above or a new query like needRelocationsForTableEvaluationData() for it.

I think we should stick to queries and have two separate queries since they're really straight forward, not too painful to add, and allow for some finer granularity and codegen flexibility.

[dchopra001]

The way we get that address into a register is by generating a label prior to the table and then calling fixedLoadLabelAddressIntoReg() which will call addMetaDataFor64BitFixedLoadLabelAddressIntoReg() which will check compileRelocatableCode().

The JITServer change in addMetaDataFor64BitFixedLoadLabelAddressIntoReg is open in a separate PR here: #4444

I will copy your suggestion there so we can keep track of it.

[dchopra001]

@ymanton This one is ready for another round of review.

[ymanton]

Looks reasonable.

[dchopra001]

@ymanton Thanks for the review! Once you're able to review eclipse-openj9/openj9#7796 and eclipse-openj9/openj9#7793, all three of these can be merged simultaneously.

[dchopra001]

@ymanton New overload defined and is now being used everywhere in OMR. PTAL.

[ymanton]

LGTM.

[ymanton]

Can you update the description and mention that it is now an overload of the existing function and is intended to be temporary?

[dchopra001]

Can you update the description and mention that it is now an overload of the existing function and is intended to be temporary?

Done.

[ymanton]

LGTM.

[fjeremic]

@genie-omr build all

[dchopra001]

The arm failure is:

15:32:17  /usr/lib/ccache//g++: 1246: /usr/lib/ccache//g++: 2be0450f937de85e54acad1e6ff1704299630a341be0f34b85c58c7d1ac980c9: not found../../omrmakefiles/rules.mk:379: recipe for target 'TDFParser' failed
15:32:17  make[2]: *** [TDFParser] Error 127
15:32:17  make[2]: Leaving directory '/home/jenkins/workspace/PullRequest-linux_arm/tools/tracegen'
15:32:17  
15:32:17  /usr/lib/ccache//g++: 1247: /usr/lib/ccache//g++: f046d3203c0c0f4c0484fa14cece99e4eeb528bb1762b4c1895b7cdb898908b9: not found
15:32:17  /usr/lib/ccache//g++: 1248: /usr/lib/ccache//g++: a68b4a3d109b19ce92c5d78effba22a4d73085687b2681f1d8074e27faa5a206: not found
15:32:17  /usr/lib/ccache//g++: 1249: /usr/lib/ccache//g++: GNUmakefile:304: recipe for target 'tools/tracegen' failed
15:32:17  05a25c8bf3025b0e7019eaf53237c2661e817814004ad15fb30f3fd271a0302b: not found
15:32:17  make[1]: *** [tools/tracegen] Error 2

This doesn't look related to the PR here.

The x86 failure also looks unrelated as it looks like a cmake configuration issue:

15:34:09    CMake will not be able to correctly generate this project.
15:34:09  Call Stack (most recent call first):
15:34:09    CMakeLists.txt:32 (project)
15:34:09  
15:34:09  
15:34:09  -- Configuring incomplete, errors occurred!
15:34:09  See also "/home/jenkins/workspace/PullRequest-linux_x86/build/CMakeFiles/CMakeOutput.log".
15:34:09  See also "/home/jenkins/workspace/PullRequest-linux_x86/build/CMakeFiles/CMakeError.log".
[Pipeline] }

The other tests passed.

@fjeremic FYI. Not sure if we should to run the failing builds again or if we can ignore them.

[fjeremic]

Changes are Power related so I'm confident in the builds which did pass. Given the infrastructure issues lately I'm ok with the current level of testing in this PR.