[SLP][NFC] Avoid the getRdxOperand hack #148672
Open
+32
−47
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We visit two operands of TreeN: - 1, 2 if isCmpSelMinMax(TreeN) - 0, 2 if TreeN is (select X, true, Y) - 0, 1 otherwise in the reverse order. Replace the loop in CheckOperands and a substitution of the second index in getRdxOperand with simply calling the code for the two determined indexes.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Piotr Fusik (pfusik) ChangesWe visit two operands of TreeN:
in the reverse order. Replace the loop in CheckOperands and a substitution of the second index Full diff: https://github.com/llvm/llvm-project/pull/148672.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index c61e1135524b6..1b78e79b6b419 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -21791,16 +21791,6 @@ class HorizontalReduction {
return I->isAssociative();
}
- static Value *getRdxOperand(Instruction *I, unsigned Index) {
- // Poison-safe 'or' takes the form: select X, true, Y
- // To make that work with the normal operand processing, we skip the
- // true value operand.
- // TODO: Change the code and data structures to handle this without a hack.
- if (getRdxKind(I) == RecurKind::Or && isa<SelectInst>(I) && Index == 1)
- return I->getOperand(2);
- return I->getOperand(Index);
- }
-
/// Creates reduction operation with the current opcode.
static Value *createOp(IRBuilderBase &Builder, RecurKind Kind, Value *LHS,
Value *RHS, const Twine &Name, bool UseSelect) {
@@ -21992,11 +21982,6 @@ class HorizontalReduction {
}
private:
- /// Total number of operands in the reduction operation.
- static unsigned getNumberOfOperands(Instruction *I) {
- return isCmpSelMinMax(I) ? 3 : 2;
- }
-
/// Checks if the instruction is in basic block \p BB.
/// For a cmp+sel min/max reduction check that both ops are in \p BB.
static bool hasSameParent(Instruction *I, BasicBlock *BB) {
@@ -22102,31 +22087,27 @@ class HorizontalReduction {
// Checks if the operands of the \p TreeN instruction are also reduction
// operations or should be treated as reduced values or an extra argument,
// which is not part of the reduction.
- auto CheckOperands = [&](Instruction *TreeN,
- SmallVectorImpl<Value *> &PossibleReducedVals,
- SmallVectorImpl<Instruction *> &ReductionOps,
- unsigned Level) {
- for (int I : reverse(seq<int>(getFirstOperandIndex(TreeN),
- getNumberOfOperands(TreeN)))) {
- Value *EdgeVal = getRdxOperand(TreeN, I);
- ReducedValsToOps[EdgeVal].push_back(TreeN);
- auto *EdgeInst = dyn_cast<Instruction>(EdgeVal);
- // If the edge is not an instruction, or it is different from the main
- // reduction opcode or has too many uses - possible reduced value.
- // Also, do not try to reduce const values, if the operation is not
- // foldable.
- if (!EdgeInst || Level > RecursionMaxDepth ||
- getRdxKind(EdgeInst) != RdxKind ||
- IsCmpSelMinMax != isCmpSelMinMax(EdgeInst) ||
- !hasRequiredNumberOfUses(IsCmpSelMinMax, EdgeInst) ||
- !isVectorizable(RdxKind, EdgeInst) ||
- (R.isAnalyzedReductionRoot(EdgeInst) &&
- all_of(EdgeInst->operands(), IsaPred<Constant>))) {
- PossibleReducedVals.push_back(EdgeVal);
- continue;
- }
+ auto CheckOperand = [&](Instruction *TreeN, int OperandIndex,
+ SmallVectorImpl<Value *> &PossibleReducedVals,
+ SmallVectorImpl<Instruction *> &ReductionOps,
+ unsigned Level) {
+ Value *EdgeVal = TreeN->getOperand(OperandIndex);
+ ReducedValsToOps[EdgeVal].push_back(TreeN);
+ auto *EdgeInst = dyn_cast<Instruction>(EdgeVal);
+ // If the edge is not an instruction, or it is different from the main
+ // reduction opcode or has too many uses - possible reduced value.
+ // Also, do not try to reduce const values, if the operation is not
+ // foldable.
+ if (!EdgeInst || Level > RecursionMaxDepth ||
+ getRdxKind(EdgeInst) != RdxKind ||
+ IsCmpSelMinMax != isCmpSelMinMax(EdgeInst) ||
+ !hasRequiredNumberOfUses(IsCmpSelMinMax, EdgeInst) ||
+ !isVectorizable(RdxKind, EdgeInst) ||
+ (R.isAnalyzedReductionRoot(EdgeInst) &&
+ all_of(EdgeInst->operands(), IsaPred<Constant>)))
+ PossibleReducedVals.push_back(EdgeVal);
+ else
ReductionOps.push_back(EdgeInst);
- }
};
// Try to regroup reduced values so that it gets more profitable to try to
// reduce them. Values are grouped by their value ids, instructions - by
@@ -22177,7 +22158,12 @@ class HorizontalReduction {
auto [TreeN, Level] = Worklist.pop_back_val();
SmallVector<Value *> PossibleRedVals;
SmallVector<Instruction *> PossibleReductionOps;
- CheckOperands(TreeN, PossibleRedVals, PossibleReductionOps, Level);
+ int I1 = getFirstOperandIndex(TreeN);
+ int I2 = isa<SelectInst>(TreeN) && match(TreeN, m_LogicalOr())
+ ? 2 // Skip "true" in "select X, true, Y"
+ : I1 + 1;
+ CheckOperand(TreeN, I2, PossibleRedVals, PossibleReductionOps, Level);
+ CheckOperand(TreeN, I1, PossibleRedVals, PossibleReductionOps, Level);
addReductionOps(TreeN);
// Add reduction values. The values are sorted for better vectorization
// results.
@@ -22295,15 +22281,14 @@ class HorizontalReduction {
isGuaranteedNotToBePoison(VectorizedTree, AC) ||
(It != ReducedValsToOps.end() &&
any_of(It->getSecond(), [&](Instruction *I) {
- return isBoolLogicOp(I) &&
- getRdxOperand(I, 0) == VectorizedTree;
+ return isBoolLogicOp(I) && I->getOperand(0) == VectorizedTree;
}))) {
;
} else if (isGuaranteedNotToBePoison(Res, AC) ||
(It1 != ReducedValsToOps.end() &&
- any_of(It1->getSecond(), [&](Instruction *I) {
- return isBoolLogicOp(I) && getRdxOperand(I, 0) == Res;
- }))) {
+ any_of(It1->getSecond(), [&](Instruction *I) {
+ return isBoolLogicOp(I) && I->getOperand(0) == Res;
+ }))) {
std::swap(VectorizedTree, Res);
} else {
VectorizedTree = Builder.CreateFreeze(VectorizedTree);
@@ -22786,11 +22771,11 @@ class HorizontalReduction {
if (!AnyBoolLogicOp)
return;
if (isBoolLogicOp(RedOp1) && ((!InitStep && LHS == VectorizedTree) ||
- getRdxOperand(RedOp1, 0) == LHS ||
+ RedOp1->getOperand(0) == LHS ||
isGuaranteedNotToBePoison(LHS, AC)))
return;
if (isBoolLogicOp(RedOp2) && ((!InitStep && RHS == VectorizedTree) ||
- getRdxOperand(RedOp2, 0) == RHS ||
+ RedOp2->getOperand(0) == RHS ||
isGuaranteedNotToBePoison(RHS, AC))) {
std::swap(LHS, RHS);
return;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I would keep the original functions. Maybe, do some early checks for index 0 and later checks for index 1, but still keep the existing abstractions. |
The comment says "TODO: Change the code and data structures to handle this without a hack." |
Yes, but you did not fix it, just moved to another place. Instead, would be good just to build the list of operands (actual operands, excluding true value) and just return the corresponding operand without adjusting the index |
The list of operands is now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We visit two operands of TreeN:
in the reverse order.
Replace the loop in CheckOperands and a substitution of the second index
in getRdxOperand with simply calling the code for the two determined indexes.