[IR] PossiblyExactOperator -> PossiblyExactInst (NFC) #72501
Conversation
Now that all the exact opcodes are no longer supported as constant expressions, rename PossiblyExactOperator to PossiblyExactInst and no longer inherit from Operator. I've kept an alias for the old name and haven't mass renamed uses in this patch for ease of review. I've opted to drop the AShrOperator and LShrOperator classes, as we don't usually add Instruction subclasses for individual binary operators and these were barely used anyway.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesNow that all the exact opcodes are no longer supported as constant expressions, rename PossiblyExactOperator to PossiblyExactInst and no longer inherit from Operator. I've kept an alias for the old name and haven't mass renamed uses in this patch for ease of review. I've opted to drop the AShrOperator and LShrOperator classes, as we don't usually add Instruction subclasses for individual binary operators and these were barely used anyway. Full diff: https://github.com/llvm/llvm-project/pull/72501.diff 9 Files Affected:
diff --git a/llvm/include/llvm/IR/ConstantFolder.h b/llvm/include/llvm/IR/ConstantFolder.h
index c2b30a65e32e251..6671d5f3314eb4c 100644
--- a/llvm/include/llvm/IR/ConstantFolder.h
+++ b/llvm/include/llvm/IR/ConstantFolder.h
@@ -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;
diff --git a/llvm/include/llvm/IR/InstrTypes.h b/llvm/include/llvm/IR/InstrTypes.h
index fc5e228168a058b..b4b58bccd94dcf0 100644
--- a/llvm/include/llvm/IR/InstrTypes.h
+++ b/llvm/include/llvm/IR/InstrTypes.h
@@ -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 {
+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
//===----------------------------------------------------------------------===//
diff --git a/llvm/include/llvm/IR/Operator.h b/llvm/include/llvm/IR/Operator.h
index 12733529a74dc7d..306d3b53e7589ee 100644
--- a/llvm/include/llvm/IR/Operator.h
+++ b/llvm/include/llvm/IR/Operator.h
@@ -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 {
@@ -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;
diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h
index f709a5ac52a4125..ffb1b4491015672 100644
--- a/llvm/include/llvm/IR/PatternMatch.h
+++ b/llvm/include/llvm/IR/PatternMatch.h
@@ -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;
}
};
diff --git a/llvm/lib/Analysis/DemandedBits.cpp b/llvm/lib/Analysis/DemandedBits.cpp
index c5017bf52498e44..0429df87480a6ac 100644
--- a/llvm/lib/Analysis/DemandedBits.cpp
+++ b/llvm/lib/Analysis/DemandedBits.cpp
@@ -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);
}
}
@@ -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);
}
}
diff --git a/llvm/lib/Analysis/VectorUtils.cpp b/llvm/lib/Analysis/VectorUtils.cpp
index 4f5db8b7aaf746f..27e1f8a1c51add4 100644
--- a/llvm/lib/Analysis/VectorUtils.cpp
+++ b/llvm/lib/Analysis/VectorUtils.cpp
@@ -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;
diff --git a/llvm/lib/IR/Constants.cpp b/llvm/lib/IR/Constants.cpp
index bc55d5b48527124..5c7af2ff3b961cb 100644
--- a/llvm/lib/IR/Constants.cpp
+++ b/llvm/lib/IR/Constants.cpp
@@ -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;
}
}
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 7449692f05d7bf9..188338bdc5c594c 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -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");
+ SubclassOptionalData = (SubclassOptionalData & ~PossiblyExactInst::IsExact) |
+ (b * PossiblyExactInst::IsExact);
}
void Instruction::setNonNeg(bool b) {
@@ -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:
@@ -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) {
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 9bc84c7dd6e1539..f61d70406c3ac9b 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -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;
|
| @@ -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 { | |||
There was a problem hiding this comment.
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.
| @@ -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"); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I'd mildly prefer the other form just for consistency, but this is very definitely non-blocking.
There was a problem hiding this comment.
LGTM. I'm fine with the name change, thanks for keeping the alias for now.
I don't really have an opinion on the setExact implementation itself. As long as there's an Instruction::setExact, it may as well be implement there directly. If anything, I'd question whether Instruction::setExact should exist in the first place. I think there's a good argument that there should only be a PossiblyExactInst::setExact method -- though I expect making that change would require more changes elsewhere.
Yeah, I agree that having the method only on PossiblyExactInst etc would be cleaner. I expect the reason that we currently put these on Instruction instead is pragmatic: We usually do not use specific subclasses for individual operators, so e.g. if you do a |
Now that all the exact opcodes are no longer supported as constant expressions, rename PossiblyExactOperator to PossiblyExactInst and no longer inherit from Operator.
I've kept an alias for the old name and haven't mass renamed uses in this patch for ease of review.
I've opted to drop the AShrOperator and LShrOperator classes, as we don't usually add Instruction subclasses for individual binary operators and these were barely used anyway.