RSInstructions encoding fix for IBM Z - #7965
Conversation
r30shah
left a comment
r30shah
left a comment
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.
I am investigating this part
There was a problem hiding this comment.
Added getOpCode().usesRegPairForTarget() check. Please let me know if I need to modify anything else.
aff3b9f to
c8be5d5
Compare
Changed commit message as suggested. Please let me know if I should add anymore details. |
ishitaR88
changed the title
c8be5d5 to
a5c0fdd
Compare
| 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.
a5c0fdd to
d18045d
Compare
r30shah
left a comment
r30shah
left a comment
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.
d18045d to
59b942c
Compare
Changed the commit message please let me know if it's alright. |
|
Jenkins build zos,zlinux |
hzongaro
left a comment
hzongaro
left a comment
There was a problem hiding this comment.
I think the change looks fine, but I wanted to point out one more issue with the commit message. According to https://github.com/eclipse-omr/omr/blob/master/CONTRIBUTING.md#commit-guidelines, the first line of commit messages should be written in the imperative mood. May I ask you to change "RSInstructions encoding fix for IBM Z" to something like "Fix encoding of RSInstructions for IBM Z"?
Also, the description of the fix contains a link to a particular line in compiler/z/codegen/S390Instruction.cpp on the master branch. However, those line numbers will change whenever someone makes changes to that file, making it difficult for anyone who looks at this pull request in the future to figure out which line it was referring to.
I would suggest either replacing that link with the name of the method that's involved, or using a link based on the most recent commit to OMR — i.e., https://github.com/eclipse-omr/omr/blob/0fba1d563edc52877796d12d1121f68cc2c265f7/compiler/z/codegen/S390Instruction.cpp#L2144
This commit fixes the encoding issue reported in eclipse-omr#5175 for RS instructions on IBM Z. The second register of the register pair was previously encoded irrespective of if it needed and it might cause some unexpected behavior in future. This fix will skip encoding the second register for the opcodes that do not use it. Issue:eclipse-omr#5175
59b942c to
0b3d985
Compare
Changed the commit message as per your suggestion. Please let me know if I need to change anything else.
-- Added the function name and class path instead of using link. |
hzongaro
left a comment
hzongaro
left a comment
There was a problem hiding this comment.
I think the changes look good. Thanks!
|
Jenkins build all |
|
Test was successful and reviews approved. Merging. |
5 participants
Before this fix the r+1 register was getting encoded by
uint8_t *TR::S390RSInstruction::generateBinaryEncoding()in~/omr/compiler/z/codegen/S390Instruction.cppas 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.