AArch64: Implement large offset path for loadaddrEvaluator() #4718
Conversation
|
Need to consider the case where the target register and the base register of |
This commit implements the path in AArch64 OMRMemoryReference for handling offsets wider than 12 bits. It is used by loadaddrEvaluator(). Signed-off-by: KONNO Kazuhiro <konno@jp.ibm.com>
knn-k
force-pushed
the
aarch64memref15
branch
from
5649e62
to
4322c5a
Compare
knn-k
changed the title
| if (needSpill) | ||
| { | ||
| immreg = cg->machine()->getRealRegister( | ||
| (base->getRegisterNumber() == TR::RealRegister::x12) ? TR::RealRegister::x11 : TR::RealRegister::x12 |
There was a problem hiding this comment.
Is there some significance to these particular registers, or are they just arbitrarily chosen?
There was a problem hiding this comment.
No particular reason. I chose them from volatile registers that are not used for method arguments.
| cursor += ARM64_INSTRUCTION_LENGTH; | ||
| } | ||
| else | ||
| { | ||
| TR_ASSERT_FATAL(false, "Offset is too large for specified instruction."); |
There was a problem hiding this comment.
I can see this as a temporary solution to get something to work, but I'm a bit hesitant to call this the long-term solution. This code appears to make an arbitrary spilling decision without considering the actual availability of the register being spilled. Given the number of registers available on this architecture, there's probably a good chance that in many cases the register is already free in which case the spill is unneeded.
This is a problem faced by the other code generators too (realizing that a register needs to be used after register allocation/assignment has occurred) and each solves the problem differently depending on the context. In this particular situation, one way to solve it might be that when this memory reference is register assigned and it knows that there is the possibility that a register might be needed during binary encoding, it could ask the machine for the current register state and record one of the free registers just in case it might be needed. The binary encoding would use that free register without a spill, or arbitrarily spill some register if there were none available.
This PR is probably OK as an initial contribution, but I would like an issue opened to track the work to implement something that makes more careful use of registers.
There was a problem hiding this comment.
I don't like this solution very much, either. I followed what p and arm versions do.
(p) https://github.com/eclipse/omr/blob/d77a173dd7275268225917b0be3bf85ed5ef6b1f/compiler/p/codegen/OMRMemoryReference.cpp#L1406-L1429
(arm) https://github.com/eclipse/omr/blob/d77a173dd7275268225917b0be3bf85ed5ef6b1f/compiler/arm/codegen/OMRMemoryReference.cpp#L1216-L1224
There was a problem hiding this comment.
Can you open an issue as I requested above so that we can move this along please?
There was a problem hiding this comment.
I opened Issue #4862 for this.
|
@genie-omr build aarch64 |
This commit implements the path in AArch64 OMRMemoryReference for
handling offsets wider than 12 bits.
It is used by loadaddrEvaluator().
Signed-off-by: KONNO Kazuhiro konno@jp.ibm.com