-
Notifications
You must be signed in to change notification settings - Fork 392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix S390 opcode properties #6447
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for supporting the project, and congratulations on your first contribution! A project committer will shortly review your contribution. In the mean time, if you haven't had a chance please skim over the contribution guidelines which all pull requests must adhere to. If the ECA pull request check fails, have a look at the instructions for signing the ECA in the legal considerations section.
If you run into any problems our community will be happy to assist you in any way we can. There are a number of recommended ways to interact with the community. We encourage you to ask questions, or drop by to say hello.
899853c
to
5645361
Compare
A bit of context on how these properties are used, from what I've found looking through the codebase:
|
@r30shah : please review |
@@ -5670,7 +5673,7 @@ | |||
/* .minimumALS = */ OMR_PROCESSOR_S390_Z900, | |||
/* .properties = */ S390OpProp_IsStore | | |||
S390OpProp_LongDispSupported | | |||
S390OpProp_SetsOperand3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am bit curious how this flag is used in the binary encoding. As bunch of places, you replaced setsOperand3
does not even use that when instruction was constructed.
Looking at one of the constructor for RSInstruction [1], previously it was setting lreg
as source register, but with this change, it will set it as target Register.
[1].
omr/compiler/z/codegen/S390Instruction.hpp
Lines 2591 to 2594 in 4553376
if (getOpCode().setsOperand2()) | |
useTargetRegister(lreg); | |
else | |
useSourceRegister(lreg); |
5645361
to
574b4ab
Compare
Jenkins build zos,zlinux |
Closes: eclipse#6155 Signed-off-by: Spencer Comin <spencer.comin@ibm.com>
574b4ab
to
2243274
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did have incorrectly tagged properties on some of the instructions. Thanks a lot @Spencer-Comin for taking time and correcting them. Changes looks good to me.
Given that jenkins build on the PR for zLinux and z/OS passed and knowing that Spencer has put his changes through sufficient internal testing through personal builds on z/OS and LoZ, @0xdaryl Can I request you to merge this changes?
I found some s390 opcodes that had incorrect and/or missing properties when comparing with documentation.
Missing properties
IsRegCopy
LER
)LDR
)LXR
)ImplicitlyUsesGPR0
PRNO
)SetsOperand2
CDS
)CS
)STAM
)STCM
)STCTL
)STM
)CDSG
)CDSY
)CSG
)CSY
)STAMY
)STCMH
)STCMY
)STCTG
)STMG
)STMH
)STMY
)UsesM3
LEDTR
)LDXTR
)VL
)VST
)CELGBR
)CELFBR
)CDLGBR
)CDLFBR
)SingleFP
CELGBR
)STE
)STEY
)UsesM4
CELGBR
)CELFBR
)CDLGBR
)CDLFBR
)UsesRegPairForSource
STPQ
)DoubleFP
STD
)STDY
)Incorrect properties
SetsOperand3
CDS
CS
STAM
STCM
STCTL
STM
CDSG
CDSY
CSG
CSY
STAMY
STCMH
STCMY
STCTG
STMG
STMH
STMY
UsesRegPairForTarget
STPQ
SingleFP
STD
DoubleFP
STE
Closes: #6155
Signed-off-by: Spencer Comin spencer.comin@ibm.com