RSInstructions encoding fix for IBM Z #7965
Conversation
There was a problem hiding this comment.
Thanks @ishitaR88 - Couple of recommendations. This change fixes #5175 so please follow https://github.com/eclipse-omr/omr/blob/master/CONTRIBUTING.md#commit-guidelines for commit message and description guidelines.
Also this issue is not specific to z/OS only. It also applies to Linux on Z, commit title and PR title needs to be changed to IBM Z (Not z/OS). Also as we are updating this for other RSInstructions as well, update the title accordingly.
| toRealRegister(getSecondRegister()->getHighOrder())->setRegister2Field((uint32_t *)cursor); | ||
| } else if (getLastRegister()) { | ||
| toRealRegister(getLastRegister())->setRegister2Field((uint32_t *)cursor); | ||
| if (op != TR::InstOpCode::SRDA ) { |
There was a problem hiding this comment.
Thinking bit more about this, Can we use something like getOpCode().usesRegPairForTarget() to identify the instruction which uses register pair in which case we do not need to encode the lastRegister.
There was a problem hiding this comment.
I have added 3 more opcodes besides SRDA that do not need the odd register encoding
| toRealRegister(getSecondRegister()->getHighOrder())->setRegister2Field((uint32_t *)cursor); | ||
| } else if (getLastRegister()) { | ||
| toRealRegister(getLastRegister())->setRegister2Field((uint32_t *)cursor); | ||
| if (op != TR::InstOpCode::SRDA ) { |
There was a problem hiding this comment.
Can we use getOpCode().usesRegPairForTarget() to recognize cases where the instruction uses register pair for target (In which case we do not need to encode the lastRegister).
There was a problem hiding this comment.
Added getOpCode().usesRegPairForTarget() check. Please let me know if I need to modify anything else.
ishitaR88
force-pushed
the
srda-bugfix
branch
3 times, most recently
from
aff3b9f to
c8be5d5
Compare
Changed commit message as suggested. Please let me know if I should add anymore details. |
ishitaR88
changed the title
ishitaR88
force-pushed
the
srda-bugfix
branch
from
c8be5d5 to
a5c0fdd
Compare
| } else if (getLastRegister()) { | ||
| toRealRegister(getLastRegister())->setRegister2Field((uint32_t *)cursor); | ||
| } else if (!opCode.usesRegPairForTarget() && getLastRegister()) { | ||
| toRealRegister(getLastRegister())->setRegister2Field((uint32_t *)cursor); |
There was a problem hiding this comment.
Seems like an extra indentation here. Please fix this.
| toRealRegister(getSecondRegister()->getHighOrder())->setRegister2Field((uint32_t *)cursor); | ||
| } else if (getLastRegister()) { | ||
| toRealRegister(getLastRegister())->setRegister2Field((uint32_t *)cursor); | ||
| } else if (!opCode.usesRegPairForTarget() && getLastRegister()) { |
There was a problem hiding this comment.
I think this should resolve the issue, but I would appreciate going through the list of instructions (That are not checked in condition before - which will use this code (RS*_FORMAT) and ensure this is fine.
Changes overall are good - Just a sanity check would confirm. I think you should remove PR from Draft to Ready once you address those two comments.
This commit fixes the encoding issue reported in eclipse-omr#5175 for RS instructions on IBM Z. The odd register was previously encoded irrespective of if it needed and it might cause some unexpected behavior in future. This fix will skip encoding second/odd registers for the opcodes that do not need it. Issue:eclipse-omr#5175
ishitaR88
force-pushed
the
srda-bugfix
branch
from
a5c0fdd to
d18045d
Compare
There was a problem hiding this comment.
@ishitaR88 - So encoding of the second register from the pair is incorrect. So make changes to commit message / body to reflect that.
Please take a look at the Instruction type that extends RSInstruction as well to ensure that those are fine.
3 participants
Before this fix the r+1 register was getting encoded by https://github.com/eclipse-omr/omr/blob/master/compiler/z/codegen/S390Instruction.cpp#L2144 as shown below, even though it is not actually a register SRDA instruction. For now this register encoding is ignored by hardware but it can cause an invalid instruction issue in future.
In this modification the encoding of r+1 register is skipped if the instruction is SRDA, r+1 register will be 0.