Skip to content

[IR] PossiblyExactOperator -> PossiblyExactInst (NFC) #72501

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −64
Open

[IR] PossiblyExactOperator -> PossiblyExactInst (NFC) #72501

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
3 changes: 1 addition & 2 deletions llvm/include/llvm/IR/ConstantFolder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ class ConstantFolder final : public IRBuilderFolder {
auto *RC = dyn_cast<Constant>(RHS);
if (LC && RC) {
if (ConstantExpr::isDesirableBinOp(Opc))
return ConstantExpr::get(Opc, LC, RC,
IsExact ? PossiblyExactOperator::IsExact : 0);
return ConstantExpr::get(Opc, LC, RC);
return ConstantFoldBinaryInstruction(Opc, LC, RC);
}
return nullptr;
Expand Down
20 changes: 20 additions & 0 deletions llvm/include/llvm/IR/InstrTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,26 @@ class PossiblyNonNegInst : public CastInst {
}
};

/// A div or shr instruction, which can be marked as "exact",
/// indicating that no bits are destroyed.
class PossiblyExactInst : public BinaryOperator {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does make me wonder if I should just keep the old PossiblyExactOperator name and only do the hierarchy change. In that case "Operator" would now refer to "BinaryOperator" rather than "Instruction or ConstExpr". I've always found the overloaded use of this term confusing.

public:
enum { IsExact = (1 << 0) };

static bool classof(const Instruction *I) {
unsigned OpC = I->getOpcode();
return OpC == Instruction::SDiv || OpC == Instruction::UDiv ||
OpC == Instruction::AShr || OpC == Instruction::LShr;
}

static bool classof(const Value *V) {
return isa<Instruction>(V) && classof(cast<Instruction>(V));
}
};

// TODO: Drop compatibility alias.
using PossiblyExactOperator = PossiblyExactInst;

//===----------------------------------------------------------------------===//
// CmpInst Class
//===----------------------------------------------------------------------===//
Expand Down
48 changes: 0 additions & 48 deletions llvm/include/llvm/IR/Operator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,47 +124,6 @@ class OverflowingBinaryOperator : public Operator {
}
};

/// A udiv or sdiv instruction, which can be marked as "exact",
/// indicating that no bits are destroyed.
class PossiblyExactOperator : public Operator {
public:
enum {
IsExact = (1 << 0)
};

private:
friend class Instruction;
friend class ConstantExpr;

void setIsExact(bool B) {
SubclassOptionalData = (SubclassOptionalData & ~IsExact) | (B * IsExact);
}

public:
/// Test whether this division is known to be exact, with zero remainder.
bool isExact() const {
return SubclassOptionalData & IsExact;
}

static bool isPossiblyExactOpcode(unsigned OpC) {
return OpC == Instruction::SDiv ||
OpC == Instruction::UDiv ||
OpC == Instruction::AShr ||
OpC == Instruction::LShr;
}

static bool classof(const ConstantExpr *CE) {
return isPossiblyExactOpcode(CE->getOpcode());
}
static bool classof(const Instruction *I) {
return isPossiblyExactOpcode(I->getOpcode());
}
static bool classof(const Value *V) {
return (isa<Instruction>(V) && classof(cast<Instruction>(V))) ||
(isa<ConstantExpr>(V) && classof(cast<ConstantExpr>(V)));
}
};

/// Utility class for floating point operations which can have
/// information about relaxed accuracy requirements attached to them.
class FPMathOperator : public Operator {
Expand Down Expand Up @@ -360,13 +319,6 @@ class ShlOperator
: public ConcreteOperator<OverflowingBinaryOperator, Instruction::Shl> {
};

class AShrOperator
: public ConcreteOperator<PossiblyExactOperator, Instruction::AShr> {
};
class LShrOperator
: public ConcreteOperator<PossiblyExactOperator, Instruction::LShr> {
};

class GEPOperator
: public ConcreteOperator<Operator, Instruction::GetElementPtr> {
friend class GetElementPtrInst;
Expand Down
4 changes: 2 additions & 2 deletions llvm/include/llvm/IR/PatternMatch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1342,8 +1342,8 @@ template <typename SubPattern_t> struct Exact_match {
Exact_match(const SubPattern_t &SP) : SubPattern(SP) {}

template <typename OpTy> bool match(OpTy *V) {
if (auto *PEO = dyn_cast<PossiblyExactOperator>(V))
return PEO->isExact() && SubPattern.match(V);
if (auto *PEI = dyn_cast<PossiblyExactInst>(V))
return PEI->isExact() && SubPattern.match(V);
return false;
}
};
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Analysis/DemandedBits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ void DemandedBits::determineLiveOperandBits(

// If the shift is exact, then the low bits are not dead
// (they must be zero).
if (cast<LShrOperator>(UserI)->isExact())
if (UserI->isExact())
AB |= APInt::getLowBitsSet(BitWidth, ShiftAmt);
}
}
Expand All @@ -217,7 +217,7 @@ void DemandedBits::determineLiveOperandBits(

// If the shift is exact, then the low bits are not dead
// (they must be zero).
if (cast<AShrOperator>(UserI)->isExact())
if (UserI->isExact())
AB |= APInt::getLowBitsSet(BitWidth, ShiftAmt);
}
}
Expand Down
6 changes: 2 additions & 4 deletions llvm/lib/Analysis/VectorUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,13 +674,11 @@ llvm::computeMinimumValueSizes(ArrayRef<BasicBlock *> Blocks, DemandedBits &DB,

// If any of M's operands demand more bits than MinBW then M cannot be
// performed safely in MinBW.
if (any_of(MI->operands(), [&DB, MinBW](Use &U) {
if (any_of(MI->operands(), [&DB, MinBW, MI](Use &U) {
auto *CI = dyn_cast<ConstantInt>(U);
// For constants shift amounts, check if the shift would result in
// poison.
if (CI &&
isa<ShlOperator, LShrOperator, AShrOperator>(U.getUser()) &&
U.getOperandNo() == 1)
if (CI && MI->isShift() && U.getOperandNo() == 1)
return CI->uge(MinBW);
uint64_t BW = bit_width(DB.getDemandedBits(&U).getZExtValue());
return bit_ceil(BW) > MinBW;
Expand Down
2 changes: 0 additions & 2 deletions llvm/lib/IR/Constants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3244,8 +3244,6 @@ Instruction *ConstantExpr::getAsInstruction(Instruction *InsertBefore) const {
BO->setHasNoSignedWrap(SubclassOptionalData &
OverflowingBinaryOperator::NoSignedWrap);
}
if (isa<PossiblyExactOperator>(BO))
BO->setIsExact(SubclassOptionalData & PossiblyExactOperator::IsExact);
return BO;
}
}
9 changes: 6 additions & 3 deletions llvm/lib/IR/Instruction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,9 @@ void Instruction::setHasNoSignedWrap(bool b) {
}

void Instruction::setIsExact(bool b) {
cast<PossiblyExactOperator>(this)->setIsExact(b);
assert(isa<PossiblyExactInst>(this) && "Instruction must support exact flag");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The idiomatic structure for these accessors appears to be to implement them on the subclass, and then dispatch via a cast to that subclass. The way you implemented this is perfectly valid, but I think it'd be better to follow the convention just to keep the code structure analogous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, the change to the structure here was was intentional, because the old approach seemed somewhat convoluted to me, now that PossiblyExactInst is a direct descendant of Instruction. This matches the implementation of PossiblyNonNegInst now (admittedly, also added by me).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd mildly prefer the other form just for consistency, but this is very definitely non-blocking.

SubclassOptionalData = (SubclassOptionalData & ~PossiblyExactInst::IsExact) |
(b * PossiblyExactInst::IsExact);
}

void Instruction::setNonNeg(bool b) {
Expand Down Expand Up @@ -354,7 +356,7 @@ void Instruction::dropPoisonGeneratingFlags() {
case Instruction::SDiv:
case Instruction::AShr:
case Instruction::LShr:
cast<PossiblyExactOperator>(this)->setIsExact(false);
setIsExact(false);
break;

case Instruction::GetElementPtr:
Expand Down Expand Up @@ -416,7 +418,8 @@ void Instruction::dropUBImplyingAttrsAndMetadata() {
}

bool Instruction::isExact() const {
return cast<PossiblyExactOperator>(this)->isExact();
assert(isa<PossiblyExactInst>(this) && "Instruction must support exact flag");
return (SubclassOptionalData & PossiblyExactInst::IsExact) != 0;
}

void Instruction::setFast(bool B) {
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,8 @@ Instruction *InstCombinerImpl::foldICmpShrConstConst(ICmpInst &I, Value *A,
if (AP2.isZero())
return nullptr;

bool IsAShr = isa<AShrOperator>(I.getOperand(0));
bool IsAShr =
cast<Instruction>(I.getOperand(0))->getOpcode() == Instruction::AShr;
if (IsAShr) {
if (AP2.isAllOnes())
return nullptr;
Expand Down