Refactoring & cleanup of linkage properties #5836
Conversation
github-actions
bot
added
arch:power
arch:riscv
build / configure
cmake
comp:compiler
labels
| FOR_EACH_RESERVED_REGISTER(machine, _properties, | ||
| reg->setState(TR::RealRegister::Locked); | ||
| reg->setAssignedRegister(reg); | ||
| ); |
There was a problem hiding this comment.
Personally I don't find this macro useful. Similarly for other similar macros introduced here. To me it adds an extra layer of abstraction/complexity when it is merely just a for loop. I recall when I first started with the codebase and didn't have a proper editor with symbols setup and I saw the:
OMRPORT_ACCESS_FROM_OMRPORT and similar macros. I could never find what that was doing or where this magical privatePortLibrary local variable was coming from. It turns out all that macro does is declare a local variable:
And then omrsysinfo_* macros consume that local variable silently:
This is super unintuitive to me as a new contributor to the codebase. To me simply declaring the local variable and using the API by passing all arguments is much more clear. No need to peel layers of complexity to understand what is happening, i.e. "in the FOR_EACH_RESERVED_REGISTER which argument needs a comma, and which argument needs a semicolon?".
I'd say a similar principle applies here.
There was a problem hiding this comment.
Personally I don't find this macro useful.
I must say I find it useful in a sense it makes it much shorter so the body is not "burried" in copy-pasted boilerplate code (*) - it is used on several places in linkage code.
(*) like geting a TR::RealRegister* for given TR::RealRegister::RegNum while filtering by register flags.
I do share your dislike for macros (and #define of constants) and I'd like to replace this macro with range-based for-loop so one can do something like
for (auto reg : this->getReservedRegisters()) { ... }
but as of now, coding standards forbid range-based for-loops (but they're out of date, AFAIK, see #5592). Also for time reasons, I left this exercise for a future.
janvrany
force-pushed
the
pr/riscv-refactor-linkage-properties
branch
from
0c6abc7 to
6b2143f
Compare
…RESERVED_REGISTER()` macro Signed-off-by: Jan Vrany <[email protected]>
This makes them available for use in other linkages too. Signed-off-by: Jan Vrany <[email protected]>
This allows one using for-loop over registers without having to cast to int and back. Signed-off-by: Jan Vrany <[email protected]>
…ived members Linkage properties have a number of members whose values can be derived from register flags. This commit introduces a new method initialize() that computes these values based on _registerFlags. Linkages are expected to fill in register flags and then call initialize() to finish properties initialization. Signed-off-by: Jan Vrany <[email protected]>
… class This commit introduces a new class, RVSystemLinkageProperties and moves all necessary initialization to its ctor (from RVSystemLinkage ctor). This is a preparation to make linkage properties static. Signed-off-by: Jan Vrany <[email protected]>
Signed-off-by: Jan Vrany <[email protected]>
This is a preparation to common some parts of linkage properties. Signed-off-by: Jan Vrany <[email protected]>
Signed-off-by: Jan Vrany <[email protected]>
Signed-off-by: Jan Vrany <[email protected]>
This is a preparation for commoning. Signed-off-by: Jan Vrany <[email protected]>
This is a preparation to common some parts of linkage properties. Signed-off-by: Jan Vrany <[email protected]>
This commit hoists ++ operators for RegNum type from RISC-V to common code. This requires all platforms to define FirstRealRegister and LastRealRegister enum values (in RealRegisterEnum.hpp) - this commit does so only for RISC-V and POWER! Signed-off-by: Jan Vrany <[email protected]>
Signed-off-by: Jan Vrany <[email protected]>
…erties Alternate stack pointer register is not used in OMR nor in OpenJ9 and this can be removed. Signed-off-by: Jan Vrany <[email protected]>
Signed-off-by: Jan Vrany <[email protected]>
This is a preparation for commoning large chunk or linkage properties. Signed-off-by: Jan Vrany <[email protected]>
This commit reorders and regroups some POWER-specific linkage members to improve readability. Also changes type of members holding registers from uint8_t to TR::RealRegister::RegNum - this makes it more obvious when debugging (GDB prints the symbolic name rather than numerical value) Signed-off-by: Jan Vrany <[email protected]>
It's meaning is not documented and bit unclear, the aim of this commit is to make a note an revisit later. Signed-off-by: Jan Vrany <[email protected]>
…ived members This commit follows similar commit in RISC-V. However, the resulting linkage properties differ in few ways: * gr11 is no longer among _argumentRegisters[] array. This seems to be some POWER hack, see @ymanton's explanation [1]. * the value of _firstAllocatableIntegerRegisters (and ..Float... and ...Vector...) is different - it is now the first argument register as per ABI. Why it was different is unclear. [1]: https://eclipse-omr.slack.com/archives/CBSE78L4U/p1614372165001600 Signed-off-by: Jan Vrany <[email protected]>
janvrany
force-pushed
the
pr/riscv-refactor-linkage-properties
branch
from
6b2143f to
08534c2
Compare
|
omr-genie build aix, plinux, riscv |
|
genie-omr build aix, plinux, riscv |
Individual backends can extend this class to add their own members and/or tweak values. This commit does this only for RISC-V and POWER as POC. Signed-off-by: Jan Vrany <[email protected]>
janvrany
force-pushed
the
pr/riscv-refactor-linkage-properties
branch
from
47636d3 to
3376c96
Compare
|
genie-omr build aix, plinux |
|
@zl-wang fyi |
The aim of this draft is to serve as a starting point for upcoming OMR arch meeting #5816.
The code does the refactoring for RISC-V backend and for POWER backend (unfinished as of now, I intend push some more work), the rest is kept as it is for now.
FYI @fjeremic , @aviansie-ben (as I'm hacking POWER backend too)