Skip to content

[NFC][LLVM] Refactor rounding mode detection of constrained fp intrinsic IDs #90854

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

Merged
merged 2 commits into from
May 7, 2024
Merged

[NFC][LLVM] Refactor rounding mode detection of constrained fp intrinsic IDs #90854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e984d21
[NFC][LLVM] Refactor rounding mode detection of constrained fp intrin…
paulwalker-arm May 2, 2024
7ede84e
Use uppercase for induction variables.
paulwalker-arm May 2, 2024
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
3 changes: 1 addition & 2 deletions llvm/include/llvm/IR/IntrinsicInst.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,7 @@ class VPBinOpIntrinsic : public VPIntrinsic {
/// This is the common base class for constrained floating point intrinsics.
class ConstrainedFPIntrinsic : public IntrinsicInst {
public:
bool isUnaryOp() const;
bool isTernaryOp() const;
unsigned getNonMetadataArgCount() const;
std::optional<RoundingMode> getRoundingMode() const;
std::optional<fp::ExceptionBehavior> getExceptionBehavior() const;
bool isDefaultFPEnvironment() const;
Expand Down
4 changes: 4 additions & 0 deletions llvm/include/llvm/IR/Intrinsics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ namespace Intrinsic {
/// Floating-Point Intrinsics".
bool isConstrainedFPIntrinsic(ID QID);

/// Returns true if the intrinsic ID is for one of the "Constrained
/// Floating-Point Intrinsics" that take rounding mode metadata.
bool hasConstrainedFPRoundingModeOperand(ID QID);

/// This is a type descriptor which explains the type requirements of an
/// intrinsic. This is returned by getIntrinsicInfoTableEntries.
struct IITDescriptor {
Expand Down
7 changes: 2 additions & 5 deletions llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2053,11 +2053,8 @@ bool IRTranslator::translateConstrainedFPIntrinsic(
Flags |= MachineInstr::NoFPExcept;

SmallVector<llvm::SrcOp, 4> VRegs;
VRegs.push_back(getOrCreateVReg(*FPI.getArgOperand(0)));
if (!FPI.isUnaryOp())
VRegs.push_back(getOrCreateVReg(*FPI.getArgOperand(1)));
if (FPI.isTernaryOp())
VRegs.push_back(getOrCreateVReg(*FPI.getArgOperand(2)));
for (unsigned I = 0, E = FPI.getNonMetadataArgCount(); I != E; ++I)
VRegs.push_back(getOrCreateVReg(*FPI.getArgOperand(I)));

MIRBuilder.buildInstr(Opcode, {getOrCreateVReg(FPI)}, VRegs, Flags);
return true;
Expand Down
12 changes: 2 additions & 10 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7962,16 +7962,8 @@ void SelectionDAGBuilder::visitConstrainedFPIntrinsic(
SDValue Chain = DAG.getRoot();
SmallVector<SDValue, 4> Opers;
Opers.push_back(Chain);
if (FPI.isUnaryOp()) {
Opers.push_back(getValue(FPI.getArgOperand(0)));
} else if (FPI.isTernaryOp()) {
Opers.push_back(getValue(FPI.getArgOperand(0)));
Opers.push_back(getValue(FPI.getArgOperand(1)));
Opers.push_back(getValue(FPI.getArgOperand(2)));
} else {
Opers.push_back(getValue(FPI.getArgOperand(0)));
Opers.push_back(getValue(FPI.getArgOperand(1)));
}
for (unsigned I = 0, E = FPI.getNonMetadataArgCount(); I != E; ++I)
Opers.push_back(getValue(FPI.getArgOperand(I)));

auto pushOutChain = [this](SDValue Result, fp::ExceptionBehavior EB) {
assert(Result.getNode()->getNumValues() == 2);
Expand Down
12 changes: 12 additions & 0 deletions llvm/lib/IR/Function.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,19 @@ bool Intrinsic::isConstrainedFPIntrinsic(ID QID) {
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC:
#include "llvm/IR/ConstrainedOps.def"
#undef INSTRUCTION
return true;
default:
return false;
}
}

bool Intrinsic::hasConstrainedFPRoundingModeOperand(Intrinsic::ID QID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably use SearchableTabless and generate this function

Copy link
Collaborator Author

@paulwalker-arm paulwalker-arm May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather hold off of that for now but it does feed into my next step of adding support for target intrinsics.

Currently I'm pinning everything on IntrStrictFP, which is perhaps a mistake and I should be more explicit and introduce dedicated IntrHasRoundMetadata and IntrHasFPExpectMetadata properties?

If acceptable I can follow this patch with that refactoring? I'm already planning to generate the case blocks so most uses of ConstrainedOps.def at the IR level is removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the whole approach of having dedicated strict intrinsics with dedicated operands is a mistake, see https://discourse.llvm.org/t/thought-on-strictfp-support/71453/9

I think these would be better as some kind of optional tag on call or an operand bundle or something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for highlighting this. I'll reassess my next steps to see if I can do something better for target intrinsics rather than repeating the same mistakes as with the existing constrained ones. I had naively assumed the design had settled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think these would be better as some kind of optional tag on call or an operand bundle or something

Playing around with an implementation of this has long been on a TODO list of mine, but I've not yet had time to try it out.

switch (QID) {
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
return ROUND_MODE == 1;
#include "llvm/IR/ConstrainedOps.def"
#undef INSTRUCTION
default:
return false;
Expand Down
25 changes: 3 additions & 22 deletions llvm/lib/IR/IRBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,17 +1029,7 @@ CallInst *IRBuilderBase::CreateConstrainedFPCast(
UseFMF = FMFSource->getFastMathFlags();

CallInst *C;
bool HasRoundingMD = false;
switch (ID) {
default:
break;
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
HasRoundingMD = ROUND_MODE; \
break;
#include "llvm/IR/ConstrainedOps.def"
}
if (HasRoundingMD) {
if (Intrinsic::hasConstrainedFPRoundingModeOperand(ID)) {
Value *RoundingV = getConstrainedFPRounding(Rounding);
C = CreateIntrinsic(ID, {DestTy, V->getType()}, {V, RoundingV, ExceptV},
nullptr, Name);
Expand Down Expand Up @@ -1088,17 +1078,8 @@ CallInst *IRBuilderBase::CreateConstrainedFPCall(
llvm::SmallVector<Value *, 6> UseArgs;

append_range(UseArgs, Args);
bool HasRoundingMD = false;
switch (Callee->getIntrinsicID()) {
default:
break;
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
HasRoundingMD = ROUND_MODE; \
break;
#include "llvm/IR/ConstrainedOps.def"
}
if (HasRoundingMD)

if (Intrinsic::hasConstrainedFPRoundingModeOperand(Callee->getIntrinsicID()))
UseArgs.push_back(getConstrainedFPRounding(Rounding));
UseArgs.push_back(getConstrainedFPExcept(Except));

Expand Down
40 changes: 13 additions & 27 deletions llvm/lib/IR/IntrinsicInst.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,37 +365,23 @@ FCmpInst::Predicate ConstrainedFPCmpIntrinsic::getPredicate() const {
return getFPPredicateFromMD(getArgOperand(2));
}

bool ConstrainedFPIntrinsic::isUnaryOp() const {
switch (getIntrinsicID()) {
default:
return false;
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
return NARG == 1;
#include "llvm/IR/ConstrainedOps.def"
}
}
unsigned ConstrainedFPIntrinsic::getNonMetadataArgCount() const {
// All constrained fp intrinsics have "fpexcept" metadata.
unsigned NumArgs = arg_size() - 1;

bool ConstrainedFPIntrinsic::isTernaryOp() const {
switch (getIntrinsicID()) {
default:
return false;
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
return NARG == 3;
#include "llvm/IR/ConstrainedOps.def"
}
// Some intrinsics have "round" metadata.
if (Intrinsic::hasConstrainedFPRoundingModeOperand(getIntrinsicID()))
NumArgs -= 1;

// Compare intrinsics take their predicate as metadata.
if (isa<ConstrainedFPCmpIntrinsic>(this))
NumArgs -= 1;

return NumArgs;
}

bool ConstrainedFPIntrinsic::classof(const IntrinsicInst *I) {
switch (I->getIntrinsicID()) {
#define INSTRUCTION(NAME, NARGS, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC:
#include "llvm/IR/ConstrainedOps.def"
return true;
default:
return false;
}
return Intrinsic::isConstrainedFPIntrinsic(I->getIntrinsicID());
}

ElementCount VPIntrinsic::getStaticVectorLength() const {
Expand Down
29 changes: 13 additions & 16 deletions llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5384,11 +5384,13 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
}
#define BEGIN_REGISTER_VP_INTRINSIC(VPID, ...) case Intrinsic::VPID:
#include "llvm/IR/VPIntrinsics.def"
#undef BEGIN_REGISTER_VP_INTRINSIC
visitVPIntrinsic(cast<VPIntrinsic>(Call));
break;
#define INSTRUCTION(NAME, NARGS, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC:
#include "llvm/IR/ConstrainedOps.def"
#undef INSTRUCTION
visitConstrainedFPIntrinsic(cast<ConstrainedFPIntrinsic>(Call));
break;
case Intrinsic::dbg_declare: // llvm.dbg.declare
Expand Down Expand Up @@ -6527,19 +6529,13 @@ void Verifier::visitVPIntrinsic(VPIntrinsic &VPI) {
}

void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
unsigned NumOperands;
bool HasRoundingMD;
switch (FPI.getIntrinsicID()) {
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
NumOperands = NARG; \
HasRoundingMD = ROUND_MODE; \
break;
#include "llvm/IR/ConstrainedOps.def"
default:
llvm_unreachable("Invalid constrained FP intrinsic!");
}
unsigned NumOperands = FPI.getNonMetadataArgCount();
bool HasRoundingMD =
Intrinsic::hasConstrainedFPRoundingModeOperand(FPI.getIntrinsicID());

// Add the expected number of metadata operands.
NumOperands += (1 + HasRoundingMD);

// Compare intrinsics carry an extra predicate metadata operand.
if (isa<ConstrainedFPCmpIntrinsic>(FPI))
NumOperands += 1;
Expand All @@ -6553,8 +6549,8 @@ void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
Type *ResultTy = FPI.getType();
Check(!ValTy->isVectorTy() && !ResultTy->isVectorTy(),
"Intrinsic does not support vectors", &FPI);
}
break;
}

case Intrinsic::experimental_constrained_lround:
case Intrinsic::experimental_constrained_llround: {
Expand Down Expand Up @@ -6593,8 +6589,8 @@ void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
"Intrinsic first argument and result vector lengths must be equal",
&FPI);
}
}
break;
}

case Intrinsic::experimental_constrained_sitofp:
case Intrinsic::experimental_constrained_uitofp: {
Expand All @@ -6616,7 +6612,8 @@ void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
"Intrinsic first argument and result vector lengths must be equal",
&FPI);
}
} break;
break;
}

case Intrinsic::experimental_constrained_fptrunc:
case Intrinsic::experimental_constrained_fpext: {
Expand Down Expand Up @@ -6645,8 +6642,8 @@ void Verifier::visitConstrainedFPIntrinsic(ConstrainedFPIntrinsic &FPI) {
"Intrinsic first argument's type must be smaller than result type",
&FPI);
}
}
break;
}

default:
break;
Expand Down
14 changes: 1 addition & 13 deletions llvm/lib/Transforms/Utils/CloneFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,18 +384,6 @@ struct PruningFunctionCloner {
};
} // namespace

static bool hasRoundingModeOperand(Intrinsic::ID CIID) {
switch (CIID) {
#define INSTRUCTION(NAME, NARG, ROUND_MODE, INTRINSIC) \
case Intrinsic::INTRINSIC: \
return ROUND_MODE == 1;
#define FUNCTION INSTRUCTION
#include "llvm/IR/ConstrainedOps.def"
default:
llvm_unreachable("Unexpected constrained intrinsic id");
}
}

Instruction *
PruningFunctionCloner::cloneInstruction(BasicBlock::const_iterator II) {
const Instruction &OldInst = *II;
Expand Down Expand Up @@ -453,7 +441,7 @@ PruningFunctionCloner::cloneInstruction(BasicBlock::const_iterator II) {
// The last arguments of a constrained intrinsic are metadata that
// represent rounding mode (absents in some intrinsics) and exception
// behavior. The inlined function uses default settings.
if (hasRoundingModeOperand(CIID))
if (Intrinsic::hasConstrainedFPRoundingModeOperand(CIID))
Args.push_back(
MetadataAsValue::get(Ctx, MDString::get(Ctx, "round.tonearest")));
Args.push_back(
Expand Down