[SLP] NFC. Replace MainOp and AltOp in TreeEntry with InstructionsState. #120198
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
InstructionsState will have default constructor.
|
@llvm/pr-subscribers-llvm-transforms Author: Han-Kuan Chen (HanKuanChen) ChangesInstructionsState will have default constructor. Full diff: https://github.com/llvm/llvm-project/pull/120198.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index d967813075bb9f..2fd90137bd4432 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -836,7 +836,7 @@ class InstructionsState {
return getOpcode() == CheckedOpcode || getAltOpcode() == CheckedOpcode;
}
- InstructionsState() = delete;
+ InstructionsState() = default;
InstructionsState(Instruction *MainOp, Instruction *AltOp)
: MainOp(MainOp), AltOp(AltOp) {}
static InstructionsState invalid() { return {nullptr, nullptr}; }
@@ -2407,15 +2407,16 @@ class BoUpSLP {
}
/// Go through the instructions in VL and append their operands.
- void appendOperandsOfVL(ArrayRef<Value *> VL, Instruction *VL0) {
+ void appendOperandsOfVL(ArrayRef<Value *> VL, const InstructionsState &S) {
assert(!VL.empty() && "Bad VL");
assert((empty() || VL.size() == getNumLanes()) &&
"Expected same number of lanes");
// IntrinsicInst::isCommutative returns true if swapping the first "two"
// arguments to the intrinsic produces the same result.
constexpr unsigned IntrinsicNumOperands = 2;
- unsigned NumOperands = VL0->getNumOperands();
- ArgSize = isa<IntrinsicInst>(VL0) ? IntrinsicNumOperands : NumOperands;
+ unsigned NumOperands = S.getMainOp()->getNumOperands();
+ ArgSize = isa<IntrinsicInst>(S.getMainOp()) ? IntrinsicNumOperands
+ : NumOperands;
OpsVec.resize(NumOperands);
unsigned NumLanes = VL.size();
for (unsigned OpIdx = 0; OpIdx != NumOperands; ++OpIdx) {
@@ -2435,8 +2436,8 @@ class BoUpSLP {
// tell the inverse operations by checking commutativity.
if (isa<PoisonValue>(VL[Lane])) {
OpsVec[OpIdx][Lane] = {
- PoisonValue::get(VL0->getOperand(OpIdx)->getType()), true,
- false};
+ PoisonValue::get(S.getMainOp()->getOperand(OpIdx)->getType()),
+ true, false};
continue;
}
bool IsInverseOperation = !isCommutative(cast<Instruction>(VL[Lane]));
@@ -2549,11 +2550,12 @@ class BoUpSLP {
public:
/// Initialize with all the operands of the instruction vector \p RootVL.
- VLOperands(ArrayRef<Value *> RootVL, Instruction *VL0, const BoUpSLP &R)
+ VLOperands(ArrayRef<Value *> RootVL, const InstructionsState &S,
+ const BoUpSLP &R)
: TLI(*R.TLI), DL(*R.DL), SE(*R.SE), R(R),
- L(R.LI->getLoopFor((VL0->getParent()))) {
+ L(R.LI->getLoopFor(S.getMainOp()->getParent())) {
// Append all the operands of RootVL.
- appendOperandsOfVL(RootVL, VL0);
+ appendOperandsOfVL(RootVL, S);
}
/// \Returns a value vector with the operands across all lanes for the
@@ -3317,9 +3319,7 @@ class BoUpSLP {
/// reordering of operands during buildTree_rec() and vectorizeTree().
SmallVector<ValueList, 2> Operands;
- /// The main/alternate instruction.
- Instruction *MainOp = nullptr;
- Instruction *AltOp = nullptr;
+ InstructionsState S;
/// Interleaving factor for interleaved loads Vectorize nodes.
unsigned InterleaveFactor = 0;
@@ -3343,10 +3343,10 @@ class BoUpSLP {
/// Set this bundle's operand from Scalars.
void setOperand(const BoUpSLP &R, bool RequireReorder = false) {
- VLOperands Ops(Scalars, MainOp, R);
+ VLOperands Ops(Scalars, S, R);
if (RequireReorder)
Ops.reorder();
- for (unsigned I : seq<unsigned>(MainOp->getNumOperands()))
+ for (unsigned I : seq<unsigned>(S.getMainOp()->getNumOperands()))
setOperand(I, Ops.getVL(I));
}
@@ -3379,13 +3379,9 @@ class BoUpSLP {
}
/// Some of the instructions in the list have alternate opcodes.
- bool isAltShuffle() const { return MainOp != AltOp; }
+ bool isAltShuffle() const { return S.isAltShuffle(); }
- bool isOpcodeOrAlt(Instruction *I) const {
- unsigned CheckedOpcode = I->getOpcode();
- return (getOpcode() == CheckedOpcode ||
- getAltOpcode() == CheckedOpcode);
- }
+ bool isOpcodeOrAlt(Instruction *I) const { return S.isOpcodeOrAlt(I); }
/// Chooses the correct key for scheduling data. If \p Op has the same (or
/// alternate) opcode as \p OpValue, the key is \p Op. Otherwise the key is
@@ -3394,30 +3390,19 @@ class BoUpSLP {
auto *I = dyn_cast<Instruction>(Op);
if (I && isOpcodeOrAlt(I))
return Op;
- return MainOp;
+ return S.getMainOp();
}
- void setOperations(const InstructionsState &S) {
- MainOp = S.getMainOp();
- AltOp = S.getAltOp();
- }
+ void setOperations(const InstructionsState &S) { this->S = S; }
- Instruction *getMainOp() const {
- return MainOp;
- }
+ Instruction *getMainOp() const { return S.getMainOp(); }
- Instruction *getAltOp() const {
- return AltOp;
- }
+ Instruction *getAltOp() const { return S.getAltOp(); }
/// The main/alternate opcodes for the list of instructions.
- unsigned getOpcode() const {
- return MainOp ? MainOp->getOpcode() : 0;
- }
+ unsigned getOpcode() const { return S.getOpcode(); }
- unsigned getAltOpcode() const {
- return AltOp ? AltOp->getOpcode() : 0;
- }
+ unsigned getAltOpcode() const { return S.getAltOpcode(); }
/// When ReuseReorderShuffleIndices is empty it just returns position of \p
/// V within vector of Scalars. Otherwise, try to remap on its reuse index.
@@ -3514,13 +3499,13 @@ class BoUpSLP {
break;
}
dbgs() << "MainOp: ";
- if (MainOp)
- dbgs() << *MainOp << "\n";
+ if (S.getMainOp())
+ dbgs() << *S.getMainOp() << "\n";
else
dbgs() << "NULL\n";
dbgs() << "AltOp: ";
- if (AltOp)
- dbgs() << *AltOp << "\n";
+ if (S.getAltOp())
+ dbgs() << *S.getAltOp() << "\n";
else
dbgs() << "NULL\n";
dbgs() << "VectorizedValue: ";
@@ -8561,7 +8546,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
LLVM_DEBUG(dbgs() << "SLP: added a vector of compares.\n");
ValueList Left, Right;
- VLOperands Ops(VL, VL0, *this);
+ VLOperands Ops(VL, S, *this);
if (cast<CmpInst>(VL0)->isCommutative()) {
// Commutative predicate - collect + sort operands of the instructions
// so that each side is more likely to have the same opcode.
|
|
@llvm/pr-subscribers-vectorizers Author: Han-Kuan Chen (HanKuanChen) ChangesInstructionsState will have default constructor. Full diff: https://github.com/llvm/llvm-project/pull/120198.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
index d967813075bb9f..2fd90137bd4432 100644
--- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
@@ -836,7 +836,7 @@ class InstructionsState {
return getOpcode() == CheckedOpcode || getAltOpcode() == CheckedOpcode;
}
- InstructionsState() = delete;
+ InstructionsState() = default;
InstructionsState(Instruction *MainOp, Instruction *AltOp)
: MainOp(MainOp), AltOp(AltOp) {}
static InstructionsState invalid() { return {nullptr, nullptr}; }
@@ -2407,15 +2407,16 @@ class BoUpSLP {
}
/// Go through the instructions in VL and append their operands.
- void appendOperandsOfVL(ArrayRef<Value *> VL, Instruction *VL0) {
+ void appendOperandsOfVL(ArrayRef<Value *> VL, const InstructionsState &S) {
assert(!VL.empty() && "Bad VL");
assert((empty() || VL.size() == getNumLanes()) &&
"Expected same number of lanes");
// IntrinsicInst::isCommutative returns true if swapping the first "two"
// arguments to the intrinsic produces the same result.
constexpr unsigned IntrinsicNumOperands = 2;
- unsigned NumOperands = VL0->getNumOperands();
- ArgSize = isa<IntrinsicInst>(VL0) ? IntrinsicNumOperands : NumOperands;
+ unsigned NumOperands = S.getMainOp()->getNumOperands();
+ ArgSize = isa<IntrinsicInst>(S.getMainOp()) ? IntrinsicNumOperands
+ : NumOperands;
OpsVec.resize(NumOperands);
unsigned NumLanes = VL.size();
for (unsigned OpIdx = 0; OpIdx != NumOperands; ++OpIdx) {
@@ -2435,8 +2436,8 @@ class BoUpSLP {
// tell the inverse operations by checking commutativity.
if (isa<PoisonValue>(VL[Lane])) {
OpsVec[OpIdx][Lane] = {
- PoisonValue::get(VL0->getOperand(OpIdx)->getType()), true,
- false};
+ PoisonValue::get(S.getMainOp()->getOperand(OpIdx)->getType()),
+ true, false};
continue;
}
bool IsInverseOperation = !isCommutative(cast<Instruction>(VL[Lane]));
@@ -2549,11 +2550,12 @@ class BoUpSLP {
public:
/// Initialize with all the operands of the instruction vector \p RootVL.
- VLOperands(ArrayRef<Value *> RootVL, Instruction *VL0, const BoUpSLP &R)
+ VLOperands(ArrayRef<Value *> RootVL, const InstructionsState &S,
+ const BoUpSLP &R)
: TLI(*R.TLI), DL(*R.DL), SE(*R.SE), R(R),
- L(R.LI->getLoopFor((VL0->getParent()))) {
+ L(R.LI->getLoopFor(S.getMainOp()->getParent())) {
// Append all the operands of RootVL.
- appendOperandsOfVL(RootVL, VL0);
+ appendOperandsOfVL(RootVL, S);
}
/// \Returns a value vector with the operands across all lanes for the
@@ -3317,9 +3319,7 @@ class BoUpSLP {
/// reordering of operands during buildTree_rec() and vectorizeTree().
SmallVector<ValueList, 2> Operands;
- /// The main/alternate instruction.
- Instruction *MainOp = nullptr;
- Instruction *AltOp = nullptr;
+ InstructionsState S;
/// Interleaving factor for interleaved loads Vectorize nodes.
unsigned InterleaveFactor = 0;
@@ -3343,10 +3343,10 @@ class BoUpSLP {
/// Set this bundle's operand from Scalars.
void setOperand(const BoUpSLP &R, bool RequireReorder = false) {
- VLOperands Ops(Scalars, MainOp, R);
+ VLOperands Ops(Scalars, S, R);
if (RequireReorder)
Ops.reorder();
- for (unsigned I : seq<unsigned>(MainOp->getNumOperands()))
+ for (unsigned I : seq<unsigned>(S.getMainOp()->getNumOperands()))
setOperand(I, Ops.getVL(I));
}
@@ -3379,13 +3379,9 @@ class BoUpSLP {
}
/// Some of the instructions in the list have alternate opcodes.
- bool isAltShuffle() const { return MainOp != AltOp; }
+ bool isAltShuffle() const { return S.isAltShuffle(); }
- bool isOpcodeOrAlt(Instruction *I) const {
- unsigned CheckedOpcode = I->getOpcode();
- return (getOpcode() == CheckedOpcode ||
- getAltOpcode() == CheckedOpcode);
- }
+ bool isOpcodeOrAlt(Instruction *I) const { return S.isOpcodeOrAlt(I); }
/// Chooses the correct key for scheduling data. If \p Op has the same (or
/// alternate) opcode as \p OpValue, the key is \p Op. Otherwise the key is
@@ -3394,30 +3390,19 @@ class BoUpSLP {
auto *I = dyn_cast<Instruction>(Op);
if (I && isOpcodeOrAlt(I))
return Op;
- return MainOp;
+ return S.getMainOp();
}
- void setOperations(const InstructionsState &S) {
- MainOp = S.getMainOp();
- AltOp = S.getAltOp();
- }
+ void setOperations(const InstructionsState &S) { this->S = S; }
- Instruction *getMainOp() const {
- return MainOp;
- }
+ Instruction *getMainOp() const { return S.getMainOp(); }
- Instruction *getAltOp() const {
- return AltOp;
- }
+ Instruction *getAltOp() const { return S.getAltOp(); }
/// The main/alternate opcodes for the list of instructions.
- unsigned getOpcode() const {
- return MainOp ? MainOp->getOpcode() : 0;
- }
+ unsigned getOpcode() const { return S.getOpcode(); }
- unsigned getAltOpcode() const {
- return AltOp ? AltOp->getOpcode() : 0;
- }
+ unsigned getAltOpcode() const { return S.getAltOpcode(); }
/// When ReuseReorderShuffleIndices is empty it just returns position of \p
/// V within vector of Scalars. Otherwise, try to remap on its reuse index.
@@ -3514,13 +3499,13 @@ class BoUpSLP {
break;
}
dbgs() << "MainOp: ";
- if (MainOp)
- dbgs() << *MainOp << "\n";
+ if (S.getMainOp())
+ dbgs() << *S.getMainOp() << "\n";
else
dbgs() << "NULL\n";
dbgs() << "AltOp: ";
- if (AltOp)
- dbgs() << *AltOp << "\n";
+ if (S.getAltOp())
+ dbgs() << *S.getAltOp() << "\n";
else
dbgs() << "NULL\n";
dbgs() << "VectorizedValue: ";
@@ -8561,7 +8546,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
LLVM_DEBUG(dbgs() << "SLP: added a vector of compares.\n");
ValueList Left, Right;
- VLOperands Ops(VL, VL0, *this);
+ VLOperands Ops(VL, S, *this);
if (cast<CmpInst>(VL0)->isCommutative()) {
// Commutative predicate - collect + sort operands of the instructions
// so that each side is more likely to have the same opcode.
|
getMainOp() -> isInstructionsStateValid()
getMainOp() -> isInstructionsStateValid()? getMainOp(): nullptr
getOpcode() -> isInstructionsStateValid()
getOpcode() -> isInstructionsStateValid()? getOpcode(): 0
getOpcode() -> isInstructionsStateValid() && getOpcode()
isAltShuffle() -> isInstructionsStateValid() && isAltShuffle()
!isAltShuffle() -> !isInstructionsStateValid() || !isAltShuffle()
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Comment on lines
3267
to
3269
| assert( | ||
| (State == NeedToGather || S.valid()) && | ||
| "InstructionsState must be valid if the TreeEntry is not gathered."); |
There was a problem hiding this comment.
No need for assertion here
There was a problem hiding this comment.
Why? I don't see a clear connection between InstructionsState and isGather.
There was a problem hiding this comment.
It just looks strange to have this assertion here
There was a problem hiding this comment.
I think we should keep it.
There was a problem hiding this comment.
I understand what you are trying to do here, but we never do this in simple unrelated functions. This function shall check the state of the node, instead we check the state of the InstructionsState. Better to have this assertion in functions, directly related to InstructionsState, not here
There was a problem hiding this comment.
Got it. The check will be in other PR.
alexey-bataev
approved these changes
Jan 9, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/16802 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/15893 Here is the relevant piece of the build log for the reference |
HanKuanChen
added a commit
that referenced
this pull request
Jan 10, 2025
…te. (#120198) Add TreeEntry::hasState. Add assert for getTreeEntry. Remove the OpValue parameter from the canReuseExtract function. Remove the Opcode parameter from the ComputeMaxBitWidth lambda function.
|
Hi, After recommit of this patch I still see the crash |
@HanKuanChen : |
Really appreciate you report. This is fixed by 5853a03 now. |
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…te. (llvm#120198) Add TreeEntry::hasState. Add assert for getTreeEntry. Remove the OpValue parameter from the canReuseExtract function. Remove the Opcode parameter from the ComputeMaxBitWidth lambda function.
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…tionsState. (llvm#120198)" This reverts commit 760f550.
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…te. (llvm#120198) Add TreeEntry::hasState. Add assert for getTreeEntry. Remove the OpValue parameter from the canReuseExtract function. Remove the Opcode parameter from the ComputeMaxBitWidth lambda function.
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…tionsState. (llvm#120198)" This reverts commit f3d6cdc.
HanKuanChen
added a commit
to HanKuanChen/llvm-project
that referenced
this pull request
Jan 14, 2025
…te. (llvm#120198) Add TreeEntry::hasState. Add assert for getTreeEntry. Remove the OpValue parameter from the canReuseExtract function. Remove the Opcode parameter from the ComputeMaxBitWidth lambda function.
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.
Add TreeEntry::hasState.
Add assert for getTreeEntry.
Remove the OpValue parameter from the canReuseExtract function.
Remove the Opcode parameter from the ComputeMaxBitWidth lambda function.