Omr: Provide factory methods for Power MemoryReference objects

[0xdaryl]

One of the reasons the "generate" pattern (kind of like a factory pattern) was introduced into the compiler codebase several years ago was to reduce the code footprint of allocating and initializing objects by encapsulating the "new" call and the constructor call into a single function.

The Power code generator does not use these for allocating MemoryReferences, and there could be a footprint savings and code readability improvement if a factory function were used instead.

I think we should start to introduce some consistency with this pattern throughout the compiler, and for that reason I favour static create() methods added to the various classes, similar in name to the TR::Node convention.

[fjeremic]

Just putting a note down on [1] that should be handled as part of this issue.

[1] https://github.com/eclipse/omr/pull/5317#pullrequestreview-443203580

[midronij]

@gita-omr @aviansie-ben @0xdaryl It's my understanding that for each MemoryReference constructor, we need a corresponding static factory method that invokes that constructor. Is there a specific naming convention we should be using? My first instinct was to use create____() (e.g.: create(), createWithLabel(), createWithDisplacement(), createWithSymRef(), etc), similar to how it's done with TR::Node, but I just wanted to clarify if there is some other naming scheme that should be used in this case.

[aviansie-ben]

@midronij I don't know of any other naming scheme that's well-established in the codebase for this sort of thing, so I'd be in favour of using createWith* for the names.

[gita-omr]

create... looks good to me. @0xdaryl what do you think about the proposal above?

[fjeremic]

I believe @0xdaryl is away this week. I too support the create... APIs and we are working on the same naming scheme on Z, i.e. TR::MemoryReference::create*(...)

[0xdaryl]

I agree with the overloaded TR::MemoryReference::create(...) naming scheme for the purest forms of MemoryReference construction, and ::createWith... forms as appropriate that wrap additional functionality around those basic forms.

I further agree that a consistent naming scheme should be employed across all architectures (I believe Z and X use "generate..." and neither AArch64 nor RISCV use them at all).

[aviansie-ben]

@0xdaryl While I would normally agree that we should use a generic overloaded create(...) function, that unfortunately causes problems for the Power codegen due to issues with overload resolution after longer displacements were introduced. Before #5317, we had two "pure" constructor overloads, one that takes a TR::Register* and an int32_t and one that takes two TR::Register*s.

To accommodate the new 34-bit immediate field, the first constructor needed to be modified to accept an int64_t. Unfortunately, simply changing the parameter type runs into overload resolution problems when the constant 0 is passed in, since it's ambiguous whether we want the integer 0 and the single register overload or a NULL pointer and the two register overload. Previously this was fine because 0 prefers use as an int during overload resolution, but since it now needs a widening conversion to int64_t this now leads to ambiguity.

I've also tried fixing this by having two overloads for the integer displacement case (one int32_t and one int64_t), but this breaks conversions from other integral types such as int16_t. It's ambiguous whether such a value should be extended to int32_t or int64_t, which leads to a situation where it's now necessary to have an overload for every integral type we could possibly pass in.

The only solutions I've managed to come up with to this problem are:

• Avoid passing in 0 directly in the first place and instead cast it manually beforehand
• Add a template overload that catches all integral types and manually casts them to int64_t
• Use an extra (useless) parameter to distinguish the int64_t constructor (this is the hack currently used to make this work for the time being [1])
• Use differently named static factory methods for the two to distinguish them

I'd personally prefer using differently named methods for this to avoid either having to introduce extra casts, template overloads, or parameters that could cause a lot of needless confusion in the future.

[1] https://github.com/eclipse/omr/blob/e4b2caf999e85055d3538bc91aa781fa34461adf/compiler/p/codegen/OMRMemoryReference.hpp#L82-L92

[0xdaryl]

Sure, I think that's reasonable and driven by practical considerations.

[midronij]

@gita-omr @aviansie-ben Here are the PRs:
OMR: https://github.com/eclipse/omr/pull/5536, https://github.com/eclipse/omr/pull/5537
OpenJ9: https://github.com/eclipse/openj9/pull/10553