Skip to content
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.

Sign up for GitHub

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

Fast Path Math.min/max_F/D #7617

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fast Path Math.min/max_F/D #7617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions compiler/p/codegen/ControlFlowEvaluator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4569,7 +4569,7 @@ static TR::Register *generateMaxMin(TR::Node *node, TR::CodeGenerator *cg, bool
TR::InstOpCode::Mnemonic move_op = type.isIntegral() ? TR::InstOpCode::mr : TR::InstOpCode::fmr;
TR::InstOpCode::Mnemonic cmp_op;
bool two_reg = (cg->comp()->target().is32Bit() && type.isInt64());
bool check_nan = type.isFloatingPoint();
bool is_float = type.isFloatingPoint();

switch (node->getOpCodeValue())
{
Expand Down Expand Up @@ -4640,7 +4640,7 @@ static TR::Register *generateMaxMin(TR::Node *node, TR::CodeGenerator *cg, bool
start_label->setStartInternalControlFlow();
end_label->setEndInternalControlFlow();
TR::LabelSymbol *nan_label;
if (check_nan)
if (is_float)
nan_label = generateLabelSymbol(cg);
TR::RegisterDependencyConditions *dep;

Expand Down Expand Up @@ -4669,20 +4669,30 @@ static TR::Register *generateMaxMin(TR::Node *node, TR::CodeGenerator *cg, bool
else
{
generateTrg1Src2Instruction(cg, cmp_op, node, condReg, trgReg, src2Reg);
if (check_nan)
if (is_float)
{
generateConditionalBranchInstruction(cg, TR::InstOpCode::bnun, node, nan_label, condReg);
// Move the NaN which is in one of trgReg or src2Reg to trgReg by fadd
generateTrg1Src2Instruction(cg, TR::InstOpCode::fadd, node, trgReg, trgReg, src2Reg);
// Go to the nan_label for NaN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will run into functional bugs (and difficult to debug) sooner or later: since src1Reg is now used in controlFlow area, you will run into register allocation bugs under high register pressure if it is not added in dependency condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a condition, is that correct?

generateConditionalBranchInstruction(cg, TR::InstOpCode::bun, node, nan_label, condReg);

generateTrg1Src2Instruction(cg, max ? TR::InstOpCode::xsmaxdp : TR::InstOpCode::xsmindp,
node, trgReg, src1Reg, src2Reg);
generateLabelInstruction(cg, TR::InstOpCode::b, node, end_label);

generateLabelInstruction(cg, TR::InstOpCode::label, node, nan_label);
// Move the NaN which is in one of trgReg or src2Reg to trgReg by fadd
generateTrg1Src2Instruction(cg, TR::InstOpCode::fadd, node, trgReg, trgReg, src2Reg);
}
generateConditionalBranchInstruction(cg, max ? TR::InstOpCode::bge : TR::InstOpCode::ble, node, end_label, condReg);
generateTrg1Src1Instruction(cg, move_op, node, trgReg, src2Reg);

dep = new (cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 3, cg->trMemory());
else
{
generateConditionalBranchInstruction(cg, max ? TR::InstOpCode::bge : TR::InstOpCode::ble,
node, end_label, condReg);
generateTrg1Src1Instruction(cg, move_op, node, trgReg, src2Reg);
}
dep = new (cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 4, cg->trMemory());
dep->addPostCondition(condReg, TR::RealRegister::NoReg);
dep->addPostCondition(trgReg, TR::RealRegister::NoReg);
if (is_float && src1Reg != trgReg)
dep->addPostCondition(src1Reg, TR::RealRegister::NoReg);
dep->addPostCondition(src2Reg, TR::RealRegister::NoReg);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guessed it is acceptable to be a little conservative. adding src1Reg into dependency condition is not necessary for integer side, but it is not wrong either.

}
generateDepLabelInstruction(cg, TR::InstOpCode::label, node, end_label, dep);
Expand Down
4 changes: 2 additions & 2 deletions compiler/p/codegen/OMRInstOpCodeProperties.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11791,7 +11791,7 @@
/* .description = "Scalar Maximum Double-Precision", */
/* .prefix = */ 0x00000000,
/* .opcode = */ 0xF0000500,
/* .format = */ FORMAT_UNKNOWN,
/* .format = */ FORMAT_FRT_FRA_FRB,
/* .minimumALS = */ OMR_PROCESSOR_PPC_P7,
/* .properties = */ PPCOpProp_IsVSX |
PPCOpProp_DoubleFP |
Expand All @@ -11804,7 +11804,7 @@
/* .description = "Scalar Minimum Double-Precision", */
/* .prefix = */ 0x00000000,
/* .opcode = */ 0xF0000540,
/* .format = */ FORMAT_UNKNOWN,
/* .format = */ FORMAT_FRT_FRA_FRB,
/* .minimumALS = */ OMR_PROCESSOR_PPC_P7,
/* .properties = */ PPCOpProp_IsVSX |
PPCOpProp_DoubleFP |
Expand Down