-
Notifications
You must be signed in to change notification settings - Fork 396
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
Z: Support NaN values w.r.t fmax/fmin/dmax/dmin on S390 #7196
Conversation
88d35ae
to
d673ff6
Compare
Tagging @r30shah and @Spencer-Comin for review. |
Personal Build #19727 to test the changes. |
@@ -334,23 +334,61 @@ xmaxxminHelper(TR::Node* node, TR::CodeGenerator* cg, TR::InstOpCode::Mnemonic c | |||
|
|||
TR::LabelSymbol* cFlowRegionStart = generateLabelSymbol(cg); | |||
TR::LabelSymbol* cFlowRegionEnd = generateLabelSymbol(cg); | |||
//Label Symbol for handling NaN operands | |||
TR::LabelSymbol* nanLabel = generateLabelSymbol(cg); |
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.
Move the initialization of nanLabel closer to where it is used.
|
||
//Handle NaN case for doubles | ||
if (node->getOpCode().isDouble()) |
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.
Is this evaluator used for non floating type min max as well (integer , long , etc) ?
I believe we pass in the move Op and compare Op so it will generate appropriate instructions for such cases.
Current change in this PR would have NaN case for all the types whereas Integer / Long do not have a NaN.
Can you confirm? If this is true than, we need to refactor and add checks so that only Double and Float would generate NaN case.
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.
Yes, you're correct, I confirm that this evaluator is also used in specific cases of int/long MaxMin calls. Since only double and float generate NaN cases, I could try refactoring out the double/float specific code to a separate helper function. Something like this at the start of xmaxxminHelper:
`
if (node->getOpCode().isDouble() || node->getOpCode().isFloat()){
return fdmaxminHelper(node, cg, compareRROp, branchCond, moveRROp);
}
`
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.
Thanks for confirming. I think you can refactor the single function to handle this scenario. You need to guard the NaN changes with isDouble/Float query. You can tackle the common condition code early on (CC 0/1/2) first and lastly you can generate code for NaN case)
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.
@sarwat12 You still need to work on this suggestion. Evaluator in current state will allocate a register to hold NaN value for Integral type min and max node as well. Code for NaN should only be generated for float and double.
a11dcc2
to
20f4bbd
Compare
Personal Build #19902 to test new changes. |
Tagging @r30shah for review. |
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.
Please address the last comment.
0a88bfa
to
680672e
Compare
|
||
TR::RegisterDependencyConditions* deps = new (cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 2, cg); | ||
//Label Symbol for handling possible NaN operand case for float/double | ||
TR::LabelSymbol* nanLabel; |
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.
Initialize nanLabel to NULL
.
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BRC, node, cFlowRegionEnd); | ||
|
||
//Handle NaN case | ||
generateS390LabelInstruction(cg, TR::InstOpCode::label, node, nanLabel); |
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.
This is incorrect. We only generate nanLabel symbol for Double / Float case. nanLabel for other case would be uninitialized. This will fail while compiling.
genLoadLongConstant(cg, node, DOUBLE_NAN, nanReg, NULL, deps, NULL); | ||
generateRRInstruction(cg, TR::InstOpCode::LDGR, node, lhsReg, nanReg); | ||
} | ||
else |
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.
If node is not double than, it can be int, long, short, byte and float. You can not assume it will be Float only.
680672e
to
241b01f
Compare
241b01f
to
63b4dbe
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.
Minor change, overall it is good.
{ | ||
//Load immediate DOUBLE_NAN value into GPR, then move it to result FPR | ||
genLoadLongConstant(cg, node, DOUBLE_NAN, nanReg, NULL, deps, NULL); | ||
generateRRInstruction(cg, TR::InstOpCode::LDGR, node, lhsReg, nanReg); |
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.
This is the common instruction for both Double and Float, should move it out of if-else block
generateRRInstruction(cg, TR::InstOpCode::LDGR, node, lhsReg, nanReg); | ||
} | ||
deps->addPostConditionIfNotAlreadyInserted(nanReg, TR::RealRegister::AssignAny); | ||
cg->stopUsingRegister(nanReg); |
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.
Can you move this line to end of evaluator after you are attaching dependency to and of ICF label (You just added the register to dep)
7284904
to
c1f1ce1
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.
LGTM
jenkins build zos,zlinux |
@sarwat12 seems like your changes are failing on z/OS. Can you investigate? |
6f25b0b
to
2a081ce
Compare
104d0d3
to
85505c0
Compare
New instructions sequence generated from running DoubleMaxMin tests:
New instructions sequence generated from running FloatMaxMin tests:
|
@sarwat12 Is the above instruction generated for Min or Max ? Looking at the branch after |
Hi @r30shah , I was trying to take a look at the fminEvaluator which makes a call to xmaxxminHelper. It seems that fmin passes the branch condition BLR. The above instruction sequence could be for Min.
In the example case above, assuming if it is a call coming from the fminEvaluator,
I could be wrong in the assumption of Min, please let me know if this seems incorrect, then I can try to verify if this may go wrong for specific corner cases. |
Can you share the binary encoding ? I want to see the CC in the encoded instruction. |
Binary encoding of the instructions sequence generated from running FloatMaxMin tests:
|
@sarwat12 Thanks for the binary encoding, I think I was bitten incorrect name printed in the instruction selection log
There is one modification though, I would like to suggest, It can be made in the dmax/fmax/dmin/fmin evaluator. We would not have to check for anything else if the operands are equal and can skip reg - reg load as well. Can I request you to make changes as part of this PR as well? |
4f789e3
to
d499cd6
Compare
if (node->getOpCode().isDouble() || node->getOpCode().isFloat()) | ||
{ | ||
if (node->getOpCode().isDouble()) | ||
{ | ||
// If first operand is NaN, then we are done, otherwise fallthrough to move second operand as result | ||
generateRREInstruction(cg, TR::InstOpCode::LTDBR, node, lhsReg, lhsReg); | ||
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_CC3, node, cFlowRegionEnd); | ||
} | ||
else if (node->getOpCode().isFloat()) | ||
{ | ||
// If first operand is NaN, then we are done, otherwise fallthrough to move second operand as result | ||
generateRREInstruction(cg, TR::InstOpCode::LTEBR, node, lhsReg, lhsReg); | ||
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_CC3, node, cFlowRegionEnd); | ||
} | ||
} |
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 think you can simplify the code above like following.
if (node->getOpCode().isDouble() || node->getOpCode().isFloat()) | |
{ | |
if (node->getOpCode().isDouble()) | |
{ | |
// If first operand is NaN, then we are done, otherwise fallthrough to move second operand as result | |
generateRREInstruction(cg, TR::InstOpCode::LTDBR, node, lhsReg, lhsReg); | |
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_CC3, node, cFlowRegionEnd); | |
} | |
else if (node->getOpCode().isFloat()) | |
{ | |
// If first operand is NaN, then we are done, otherwise fallthrough to move second operand as result | |
generateRREInstruction(cg, TR::InstOpCode::LTEBR, node, lhsReg, lhsReg); | |
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_CC3, node, cFlowRegionEnd); | |
} | |
} | |
if (node->getOpCode().isFloatingPoint()) | |
{ | |
// If first operand is NaN, then we are done, otherwise fallthrough to move second operand as result | |
generateRREInstruction(cg, node->getOpCode().isDouble() ? TR::InstOpCode::LTDBR : TR::InstOpCode::LTEBR , node, lhsReg, lhsReg); | |
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_CC3, node, cFlowRegionEnd); | |
} |
d499cd6
to
3144615
Compare
Jenkins build zos,zlinux |
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.
One last minor nitpick. As it is just for removing comment, please wait while current builds finish before making any changes, so that we do not need to launch builds again.
@@ -338,21 +338,29 @@ xmaxxminHelper(TR::Node* node, TR::CodeGenerator* cg, TR::InstOpCode::Mnemonic c | |||
generateS390LabelInstruction(cg, TR::InstOpCode::label, node, cFlowRegionStart); | |||
cFlowRegionStart->setStartInternalControlFlow(); | |||
|
|||
//Checking common Condition Code and branching to cFlowRegionEnd |
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 do not see any relevance of this comment. It is checking for the condition passed into this common utility function. Pls remove it.
3144615
to
2a431ff
Compare
Build for zlinux: https://ci.eclipse.org/omr/job/PullRequest-linux_390-64/4344/ |
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.
LGTM. @sarwat12 Given that we still could not get this working on z/OS (Hence could not enable the test on z/OS). Can you confirm the tests are enabled on Linux on Z?
Also please open up an issue to document what we observed on z/OS to make sure we do come back to those issues.
@sarwat12 commit body for second commit should adhere to commit guideline Also, Please update the commit title to reflect both facts (Enabling on LoZ and disabling on z/OS) |
224f918
to
e25db7c
Compare
Jenkins build zos,zlinux |
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.
LGTM.
@hzongaro Can We request you to review and merge these changes? |
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 think the changes look correct. Just a small suggestion regarding comments.
Also, the commit guidelines recommend that the title of a commit be less than 70 characters, and that each line in the body of the commit be at most 72 characters, if possible. May I ask you to adjust the comments accordingly?
Finally, as tests are still failing on z/OS, please remove the "Closes: #5157" comment.
e25db7c
to
07972ab
Compare
- Enables the Float & Double MaxMin tests on Linux on Z - Disables them on z/OS, since TRIL parser cannot handle NaN values Closes: eclipse-omr#5157 Signed-off-by: Sarwat Shaheen <sarwat.shaheen@yahoo.com>
Adds support to the S390 MaxMin evaluator to handle NaN operands - Check f/d operands for NaN and set return register to NaN - Avoid extra load instructions for NaN, if operands are equal Closes: eclipse-omr#5157 Signed-off-by: Sarwat Shaheen sarwat.shaheen@yahoo.com
07972ab
to
27c1618
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.
Looks good. Thanks!
Most recent changes only added some comments and updated commit message, so no need to rerun testing. Merging. |
This PR adds support to the S390 MaxMin evaluator to handle NaN operands for [fd]min/max opcodes.
Closes: #5157
Signed-off-by: Sarwat Shaheen sarwat.shaheen@yahoo.com