Skip to content

[SLP] Normalize debug messages for newTreeEntry. #119514

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
HanKuanChen merged 5 commits into from
Dec 23, 2024
Merged

[SLP] Normalize debug messages for newTreeEntry. #119514

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6ed2af
[SLP] Normalize debug messages for newTreeEntry.
HanKuanChen Dec 9, 2024
bc6b053
alternate op->isAltShuffle
HanKuanChen Dec 11, 2024
96862bd
dump TreeEntry after newTreeEntry
HanKuanChen Dec 12, 2024
a7de688
add LLVM_DEBUG
HanKuanChen Dec 16, 2024
edccabd
apply comment
HanKuanChen Dec 18, 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
55 changes: 35 additions & 20 deletions llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3671,6 +3671,7 @@ class BoUpSLP {

if (UserTreeIdx.UserTE)
Last->UserTreeIndices.push_back(UserTreeIdx);
LLVM_DEBUG(Last->dump());
Copy link
Member

@alexey-bataev alexey-bataev Dec 16, 2024

Choose a reason for hiding this comment

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

No, make it part of debug messages, do not dump it here

return Last;
}

Expand Down Expand Up @@ -8388,7 +8389,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,

TreeEntry *TE =
newTreeEntry(VL, Bundle, S, UserTreeIdx, ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a vector of PHINodes.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (PHINode).\n");

// Keeps the reordered operands to avoid code duplication.
PHIHandler Handler(*DT, PH, VL);
Expand Down Expand Up @@ -8417,13 +8418,13 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
}
// Insert new order with initial value 0, if it does not exist,
// otherwise return the iterator to the existing one.
newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices, CurrentOrder);
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices, CurrentOrder);
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry "
"(ExtractValueInst/ExtractElementInst).\n");
// This is a special case, as it does not gather, but at the same time
// we are not extending buildTree_rec() towards the operands.
ValueList Op0;
Op0.assign(VL.size(), VL0->getOperand(0));
VectorizableTree.back()->setOperand(0, Op0);
TE->setOperand(*this);
Copy link
Contributor

@alexfh alexfh Jan 9, 2025

Choose a reason for hiding this comment

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

Was this change intended for this commit? It's not just about debug messages, IIUC.

Copy link
Contributor Author

@HanKuanChen HanKuanChen Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. This part is not related to the debug message.
Do you have any concern?

Copy link
Contributor

@alexfh alexfh Jan 10, 2025

Choose a reason for hiding this comment

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

We see test failures in code compiled by clang built after this commit. I wonder if this is related to the this snippet. While I'm working on a test case I briefly looked at this commit and found only one suspicious fragment - this one.

Copy link
Contributor Author

@HanKuanChen HanKuanChen Jan 10, 2025

Choose a reason for hiding this comment

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

Can you send me the test?

Copy link
Contributor

@asmok-g asmok-g Jan 10, 2025

Choose a reason for hiding this comment

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

In another instance, I think this is causing an infinite loop. Working on some repro

Copy link
Contributor Author

@HanKuanChen HanKuanChen Jan 14, 2025

Choose a reason for hiding this comment

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

I use the commit 539b15b as base and revert 11676da. However, reduced.ll gives me same result.

Copy link
Contributor

@alexfh alexfh Jan 14, 2025

Choose a reason for hiding this comment

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

I can reliably reproduce this and here's the same example in compiler explorer: https://gcc.godbolt.org/z/cEWPchGaE

Copy link
Contributor

@alexfh alexfh Jan 14, 2025

Choose a reason for hiding this comment

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

I mean, I could reliably reproduce this with opt built right before and at the same commit. I haven't tried it with 539b15b

Copy link
Contributor

@alexfh alexfh Jan 14, 2025

Choose a reason for hiding this comment

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

I confirmed this at b270525, but at 283dca5 this already seems to have been fixed. Searching for the fix.

Copy link
Contributor

@alexfh alexfh Jan 14, 2025

Choose a reason for hiding this comment

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

Apparently, 066b888 fixes this.

return;
}
case Instruction::InsertElement: {
Expand Down Expand Up @@ -8451,7 +8452,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
CurrentOrder.clear();
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
{}, CurrentOrder);
LLVM_DEBUG(dbgs() << "SLP: added inserts bundle.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (InsertElementInst).\n");

TE->setOperand(*this);
buildTree_rec(TE->getOperand(1), Depth + 1, {TE, 1});
Expand All @@ -8471,21 +8472,25 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices, CurrentOrder, InterleaveFactor);
if (CurrentOrder.empty())
LLVM_DEBUG(dbgs() << "SLP: added a vector of loads.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (LoadInst).\n");
else
LLVM_DEBUG(dbgs() << "SLP: added a vector of jumbled loads.\n");
LLVM_DEBUG(dbgs()
<< "SLP: added a new TreeEntry (jumbled LoadInst).\n");
break;
case TreeEntry::StridedVectorize:
// Vectorizing non-consecutive loads with `llvm.masked.gather`.
TE = newTreeEntry(VL, TreeEntry::StridedVectorize, Bundle, S,
UserTreeIdx, ReuseShuffleIndices, CurrentOrder);
LLVM_DEBUG(dbgs() << "SLP: added a vector of strided loads.\n");
LLVM_DEBUG(
dbgs() << "SLP: added a new TreeEntry (strided LoadInst).\n");
break;
case TreeEntry::ScatterVectorize:
// Vectorizing non-consecutive loads with `llvm.masked.gather`.
TE = newTreeEntry(VL, TreeEntry::ScatterVectorize, Bundle, S,
UserTreeIdx, ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a vector of non-consecutive loads.\n");
LLVM_DEBUG(
dbgs()
<< "SLP: added a new TreeEntry (non-consecutive LoadInst).\n");
break;
case TreeEntry::CombinedVectorize:
case TreeEntry::NeedToGather:
Expand Down Expand Up @@ -8529,7 +8534,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
}
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a vector of casts.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (CastInst).\n");

TE->setOperand(*this);
for (unsigned I : seq<unsigned>(VL0->getNumOperands()))
Expand All @@ -8556,7 +8561,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
CmpInst::Predicate P0 = cast<CmpInst>(VL0)->getPredicate();
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a vector of compares.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (CmpInst).\n");

ValueList Left, Right;
VLOperands Ops(VL, VL0, *this);
Expand Down Expand Up @@ -8626,7 +8631,9 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
case Instruction::Freeze: {
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a vector of un/bin op.\n");
LLVM_DEBUG(
dbgs() << "SLP: added a new TreeEntry "
"(SelectInst/UnaryOperator/BinaryOperator/FreezeInst).\n");

TE->setOperand(*this, isa<BinaryOperator>(VL0) && isCommutative(VL0));
for (unsigned I : seq<unsigned>(VL0->getNumOperands()))
Expand All @@ -8636,7 +8643,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
case Instruction::GetElementPtr: {
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a vector of GEPs.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (GetElementPtrInst).\n");
SmallVector<ValueList, 2> Operands(2);
// Prepare the operand vector for pointer operands.
for (Value *V : VL) {
Expand Down Expand Up @@ -8694,12 +8701,13 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
fixupOrderingIndices(CurrentOrder);
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices, CurrentOrder);
TE->setOperand(*this);
buildTree_rec(TE->getOperand(0), Depth + 1, {TE, 0});
if (Consecutive)
LLVM_DEBUG(dbgs() << "SLP: added a vector of stores.\n");
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (StoreInst).\n");
else
LLVM_DEBUG(dbgs() << "SLP: added a vector of jumbled stores.\n");
LLVM_DEBUG(
dbgs() << "SLP: added a new TreeEntry (jumbled StoreInst).\n");
TE->setOperand(*this);
buildTree_rec(TE->getOperand(0), Depth + 1, {TE, 0});
return;
}
case Instruction::Call: {
Expand All @@ -8710,6 +8718,7 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,

TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (CallInst).\n");
TE->setOperand(*this, isCommutative(VL0));
for (unsigned I : seq<unsigned>(CI->arg_size())) {
// For scalar operands no need to create an entry since no need to
Expand All @@ -8723,7 +8732,13 @@ void BoUpSLP::buildTree_rec(ArrayRef<Value *> VL, unsigned Depth,
case Instruction::ShuffleVector: {
TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
ReuseShuffleIndices);
LLVM_DEBUG(dbgs() << "SLP: added a ShuffleVector op.\n");
if (S.isAltShuffle()) {
LLVM_DEBUG(dbgs() << "SLP: added a new TreeEntry (isAltShuffle).\n");
} else {
assert(SLPReVec && "Only supported by REVEC.");
LLVM_DEBUG(
dbgs() << "SLP: added a new TreeEntry (ShuffleVectorInst).\n");
}

// Reorder operands if reordering would enable vectorization.
auto *CI = dyn_cast<CmpInst>(VL0);
Expand Down
Loading