Skip to content

[RISCV][GISel] Add support for G_FCMP with F and D extensions. #70624

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

Merged
merged 2 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
128 changes: 128 additions & 0 deletions llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class RISCVInstructionSelector : public InstructionSelector {
bool selectSExtInreg(MachineInstr &MI, MachineIRBuilder &MIB) const;
bool selectSelect(MachineInstr &MI, MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) const;
bool selectFPCompare(MachineInstr &MI, MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) const;

ComplexRendererFns selectShiftMask(MachineOperand &Root) const;
ComplexRendererFns selectAddrRegImm(MachineOperand &Root) const;
Expand Down Expand Up @@ -415,6 +417,8 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
}
case TargetOpcode::G_SELECT:
return selectSelect(MI, MIB, MRI);
case TargetOpcode::G_FCMP:
return selectFPCompare(MI, MIB, MRI);
default:
return false;
}
Expand Down Expand Up @@ -820,6 +824,130 @@ bool RISCVInstructionSelector::selectSelect(MachineInstr &MI,
return constrainSelectedInstRegOperands(*Result, TII, TRI, RBI);
}

// Convert an FCMP predicate to one of the supported F or D instructions.
static unsigned getFCmpOpcode(CmpInst::Predicate Pred, unsigned Size) {
assert((Size == 32 || Size == 64) && "Unsupported size");
switch (Pred) {
default:
llvm_unreachable("Unsupported predicate");
case CmpInst::FCMP_OLT:
return Size == 32 ? RISCV::FLT_S : RISCV::FLT_D;
case CmpInst::FCMP_OLE:
return Size == 32 ? RISCV::FLE_S : RISCV::FLE_D;
case CmpInst::FCMP_OEQ:
return Size == 32 ? RISCV::FEQ_S : RISCV::FEQ_D;
}
}

// Try legalizing an FCMP by swapping or inverting the predicate to one that
// is supported.
static bool legalizeFCmpPredicate(Register &LHS, Register &RHS,
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of stuff should go in LegalizerHelper. We should be able to add the cond code to the legalizer queries for G_ICMP/G_FCMP and replicate the DAG's setCondCodeAction

Choose a reason for hiding this comment

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

It should be fine to claim support for all condition codes in the legalizer. Do target independent combines and later down the pass pipeline, they can do RISCV-V specific optimizations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My immediate goal was just to get -O0 working. I will start looking at how to put this in LegalizerHelper, but I don't want that to block this patch if possible.

CmpInst::Predicate &Pred, bool &NeedInvert) {
auto isLegalFCmpPredicate = [](CmpInst::Predicate Pred) {
return Pred == CmpInst::FCMP_OLT || Pred == CmpInst::FCMP_OLE ||
Pred == CmpInst::FCMP_OEQ;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to check the case where Pred is already legal: isLegalFCmpPredicate(Pred)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that olt is getting selected correctly, but I don't see it getting handled by any of the code in this patch. How is that code getting selected? selectImpl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. tablegen has patterns for oeq, olt, and ole so selectImpl gets it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. If that’s the case, then it’s up to you if you’d like to have the assert here. I made my initial comment thinking we had to handle it with custom code.


assert(!isLegalFCmpPredicate(Pred) && "Predicate already legal?");

CmpInst::Predicate InvPred = CmpInst::getSwappedPredicate(Pred);
if (isLegalFCmpPredicate(InvPred)) {
Pred = InvPred;
std::swap(LHS, RHS);
return true;
}

InvPred = CmpInst::getInversePredicate(Pred);
NeedInvert = true;
if (isLegalFCmpPredicate(InvPred)) {
Pred = InvPred;
return true;
}
InvPred = CmpInst::getSwappedPredicate(InvPred);
if (isLegalFCmpPredicate(InvPred)) {
Pred = InvPred;
std::swap(LHS, RHS);
return true;
}

return false;
}

// Emit a sequence of instructions to compare LHS and RHS using Pred. Return
// the result in DstReg.
// FIXME: Maybe we should expand this earlier.
bool RISCVInstructionSelector::selectFPCompare(MachineInstr &MI,
MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) const {
auto &CmpMI = cast<GFCmp>(MI);
CmpInst::Predicate Pred = CmpMI.getCond();

Register DstReg = CmpMI.getReg(0);
Register LHS = CmpMI.getLHSReg();
Register RHS = CmpMI.getRHSReg();

unsigned Size = MRI.getType(LHS).getSizeInBits();
assert((Size == 32 || Size == 64) && "Unexpected size");

Register TmpReg = DstReg;

bool NeedInvert = false;
// First try swapping operands or inverting.
if (legalizeFCmpPredicate(LHS, RHS, Pred, NeedInvert)) {
if (NeedInvert)
TmpReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
auto Cmp = MIB.buildInstr(getFCmpOpcode(Pred, Size), {TmpReg}, {LHS, RHS});
if (!Cmp.constrainAllUses(TII, TRI, RBI))
return false;
} else if (Pred == CmpInst::FCMP_ONE || Pred == CmpInst::FCMP_UEQ) {
// fcmp one LHS, RHS => (OR (FLT LHS, RHS), (FLT RHS, LHS))
NeedInvert = Pred == CmpInst::FCMP_UEQ;
auto Cmp1 = MIB.buildInstr(getFCmpOpcode(CmpInst::FCMP_OLT, Size),
{&RISCV::GPRRegClass}, {LHS, RHS});
if (!Cmp1.constrainAllUses(TII, TRI, RBI))
return false;
auto Cmp2 = MIB.buildInstr(getFCmpOpcode(CmpInst::FCMP_OLT, Size),
{&RISCV::GPRRegClass}, {RHS, LHS});
if (!Cmp2.constrainAllUses(TII, TRI, RBI))
return false;
if (NeedInvert)
TmpReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
auto Or =
MIB.buildInstr(RISCV::OR, {TmpReg}, {Cmp1.getReg(0), Cmp2.getReg(0)});
if (!Or.constrainAllUses(TII, TRI, RBI))
return false;
} else if (Pred == CmpInst::FCMP_ORD || Pred == CmpInst::FCMP_UNO) {
// fcmp ord LHS, RHS => (AND (FEQ LHS, LHS), (FEQ RHS, RHS))
// FIXME: If LHS and RHS are the same we can use a single FEQ.
NeedInvert = Pred == CmpInst::FCMP_UNO;
auto Cmp1 = MIB.buildInstr(getFCmpOpcode(CmpInst::FCMP_OEQ, Size),
{&RISCV::GPRRegClass}, {LHS, LHS});
if (!Cmp1.constrainAllUses(TII, TRI, RBI))
return false;
auto Cmp2 = MIB.buildInstr(getFCmpOpcode(CmpInst::FCMP_OEQ, Size),
{&RISCV::GPRRegClass}, {RHS, RHS});
if (!Cmp2.constrainAllUses(TII, TRI, RBI))
return false;
if (NeedInvert)
TmpReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
auto And =
MIB.buildInstr(RISCV::AND, {TmpReg}, {Cmp1.getReg(0), Cmp2.getReg(0)});
if (!And.constrainAllUses(TII, TRI, RBI))
return false;
} else
llvm_unreachable("Unhandled predicate");
Comment on lines +937 to +938
Copy link
Member

Choose a reason for hiding this comment

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

style: add braces here to keep it uniform with prior if blocks


// Emit an XORI to invert the result if needed.
if (NeedInvert) {
auto Xor = MIB.buildInstr(RISCV::XORI, {DstReg}, {TmpReg}).addImm(1);
if (!Xor.constrainAllUses(TII, TRI, RBI))
return false;
}

MI.eraseFromParent();
return true;
}

namespace llvm {
InstructionSelector *
createRISCVInstructionSelector(const RISCVTargetMachine &TM,
Expand Down
8 changes: 8 additions & 0 deletions llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,14 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST) {
typeIs(1, s32)(Query));
});

getActionDefinitionsBuilder(G_FCMP)
.legalIf([=, &ST](const LegalityQuery &Query) -> bool {
return typeIs(0, sXLen)(Query) &&
((ST.hasStdExtF() && typeIs(1, s32)(Query)) ||
(ST.hasStdExtD() && typeIs(1, s64)(Query)));
})
.clampScalar(0, sXLen, sXLen);

getActionDefinitionsBuilder(G_FCONSTANT)
.legalIf([=, &ST](const LegalityQuery &Query) -> bool {
return (ST.hasStdExtF() && typeIs(0, s32)(Query)) ||
Expand Down
11 changes: 11 additions & 0 deletions llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,17 @@ RISCVRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
getOperandsMapping({getFPValueMapping(Ty.getSizeInBits()), nullptr});
break;
}
case TargetOpcode::G_FCMP: {
LLT Ty = MRI.getType(MI.getOperand(2).getReg());

unsigned Size = Ty.getSizeInBits();
assert((Size == 32 || Size == 64) && "Unsupported size for G_FCMP");

auto *FPRValueMapping = getFPValueMapping(Size);
OperandsMapping = getOperandsMapping(
{GPRValueMapping, nullptr, FPRValueMapping, FPRValueMapping});
break;
}
default:
return getInvalidInstructionMapping();
}
Expand Down
Loading