Skip to content
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

[NFC] Use reference instead of copies in few places #5118

Merged
merged 3 commits into from
Nov 18, 2024
Merged

[NFC] Use reference instead of copies in few places #5118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8d69df
Fix coverity issues
anmyachev Nov 11, 2024
5dbf3b0
Use 'ArrayRef' in 'AxisInfo' constructors
anmyachev Nov 15, 2024
e5574e5
Merge branch 'main' into coverity-fixes
anmyachev Nov 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
8 changes: 4 additions & 4 deletions lib/Analysis/Allocation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ ScratchConfig getScratchConfigForCvt(RankedTensorType srcTy,

assert(cvtNeedsSharedMemory(srcTy, dstTy));

auto inOrd = gpu::getOrder(srcLayout);
auto outOrd = gpu::getOrder(dstLayout);
const auto &inOrd = gpu::getOrder(srcLayout);
const auto &outOrd = gpu::getOrder(dstLayout);
Comment on lines +87 to +88
Copy link
Contributor

@lezcano lezcano Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you take this result by reference? Is it one of those cases where the lifetime is extended?

At any rate, a better fix would be to make getOrder have NRVO semantics, but it's pretty much a microoptimisation at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it one of those cases where the lifetime is extended?

Yes, I would also like to point out that this was suggested by https://scan.coverity.com/ tool, which we use in XPU backend mostly for our files, but since some of it is duplicated, I thought it would be nice to push even such minor changes upstream to improve the code a bit.

At any rate, a better fix would be to make getOrder have NRVO semantics, but it's pretty much a microoptimisation at this point

Unfortunately I'm not sure what needs to be done for this. Can we leave it as is until this code is considered part of the hottest path?

scratchConfig.order = outOrd;

unsigned srcContigPerThread =
Expand Down Expand Up @@ -304,7 +304,7 @@ class AllocationAnalysis {
/// arguments are involved.
void resolveAliasBufferLiveness(
function_ref<Interval<size_t>(Value value)> getLiveness) {
for (auto aliasBufferIter : allocation->aliasBuffer) {
for (const auto &aliasBufferIter : allocation->aliasBuffer) {
auto value = aliasBufferIter.first;
auto buffers = aliasBufferIter.second;
auto range = getLiveness(value);
Expand Down Expand Up @@ -444,7 +444,7 @@ class AllocationAnalysis {
std::find_if(xBuffers.begin(), xBuffers.end(), [&](auto *buffer) {
auto xRange = bufferRange[buffer];
bool res = xRange.intersects(range);
for (auto val : tripleMap)
for (const auto &val : tripleMap)
res = res &&
!val.second.intersects(xRange); // only one buffer intersect
return res;
Expand Down
43 changes: 28 additions & 15 deletions lib/Analysis/AxisInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ class BinaryOpVisitorImpl : public AxisInfoVisitorImpl<OpTy> {
divisibility.push_back(getDivisibility(op, lhsInfo, rhsInfo, d));
}
}
return AxisInfo(contiguity, divisibility, constancy, constantValue);
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy), constantValue);
}

protected:
Expand Down Expand Up @@ -541,7 +542,8 @@ class SplatOpAxisInfoVisitor final
divisibility.push_back(opInfo.getDivisibility(0));
constancy.push_back(retTy.getShape()[d]);
}
return AxisInfo(contiguity, divisibility, constancy,
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy),
operands[0]->getValue().getConstantValue());
}
};
Expand Down Expand Up @@ -572,7 +574,8 @@ class LoadOpAxisInfoVisitor final : public AxisInfoVisitorImpl<triton::LoadOp> {
maskInfo.has_value() ? maskInfo->getConstancy(d) : 0));
}

return AxisInfo(contiguity, divisibility, constancy);
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy));
}
};

Expand Down Expand Up @@ -606,7 +609,8 @@ class ExpandDimsOpAxisInfoVisitor final
contiguity.insert(contiguity.begin() + op.getAxis(), 1);
divisibility.insert(divisibility.begin() + op.getAxis(), newDivisibility);
constancy.insert(constancy.begin() + op.getAxis(), 1);
return AxisInfo(contiguity, divisibility, constancy,
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy),
operands[0]->getValue().getConstantValue());
}
};
Expand Down Expand Up @@ -635,7 +639,8 @@ class BroadcastOpAxisInfoVisitor final
constancy.push_back(opShape[d] == 1 ? retShape[d]
: opInfo.getConstancy(d));
}
return AxisInfo(contiguity, divisibility, constancy,
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy),
operands[0]->getValue().getConstantValue());
}
};
Expand Down Expand Up @@ -710,7 +715,8 @@ class CmpOpAxisInfoVisitor final : public AxisInfoVisitorImpl<OpTy> {
contiguity.push_back(1);
}

return AxisInfo(contiguity, divisibility, constancy, constantValue);
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy), constantValue);
}

private:
Expand Down Expand Up @@ -838,7 +844,8 @@ class SelectOpAxisInfoVisitor final : public AxisInfoVisitorImpl<OpTy> {
constantValue = lhsInfo.getConstantValue();
}

return AxisInfo(contiguity, divisibility, constancy, constantValue);
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy), constantValue);
}
};

Expand Down Expand Up @@ -991,7 +998,8 @@ class MaxMinOpAxisInfoVisitor final : public AxisInfoVisitorImpl<OpTy> {
contiguity.push_back(
std::min(lhsInfo.getContiguity(d), rhsInfo.getContiguity(d)));
}
return AxisInfo(contiguity, divisibility, constancy, std::nullopt);
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy), std::nullopt);
}
}
};
Expand Down Expand Up @@ -1074,8 +1082,8 @@ LogicalResult AxisInfoAnalysis::visitOperation(
auto vals = cast<DenseElementsAttr>(attr).getValues<int>();
newConstancy = AxisInfo::DimVectorT(vals.begin(), vals.end());
}
curr = AxisInfo(newContiguity, newDivisibility, newConstancy,
curr.getConstantValue());
curr = AxisInfo(std::move(newContiguity), std::move(newDivisibility),
std::move(newConstancy), curr.getConstantValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure those move semantic do much as the constructor currently takes arg by copy. Is there anything special you are trying to fix? I'm not sure this helps readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

constructor currently takes arg by copy

The AxisInfo constructor takes arguments by their value, yes, in this case SmallVector' copy constructor or the move constructor can be called (SmallVector has a move constructor). Since std::move casts the object to something like: SmallVector&&, the move constructor will be called, that is, we can get rid of one copy (for each SmallVector value).

I'm not sure this helps readability

It's a bit harder to read, I agree, but it saves three copies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the better fix here simply making the AxisInfo constructor take everything by const-ref? I think it's just an oversight, as the constructor doesn't use the "copy-move" idiom.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the better fix here simply making the AxisInfo constructor take everything by const-ref? I think it's just an oversight, as the constructor doesn't use the "copy-move" idiom.

I agree

Copy link
Contributor

Choose a reason for hiding this comment

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

Or ArrayRef<int64_t>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// join all lattice elements
for (auto *result : results)
propagateIfChanged(result, result->join(curr));
Expand All @@ -1085,15 +1093,18 @@ LogicalResult AxisInfoAnalysis::visitOperation(
void AxisInfoAnalysis::visitForOpInductionVar(
scf::ForOp op, ArrayRef<dataflow::Lattice<AxisInfo> *> argLattices) {
ProgramPoint programPoint(op);
auto lb = getLatticeElementFor(&programPoint, op.getLowerBound())->getValue();
auto step = getLatticeElementFor(&programPoint, op.getStep())->getValue();
const auto &lb =
getLatticeElementFor(&programPoint, op.getLowerBound())->getValue();
const auto &step =
getLatticeElementFor(&programPoint, op.getStep())->getValue();

AxisInfo::DimVectorT knownContiguity(1, 1);
AxisInfo::DimVectorT knownDivisibility(1, 1);
AxisInfo::DimVectorT knownConstancy(1, 1);
knownDivisibility[0] = gcd(lb.getDivisibility(0), step.getDivisibility(0));
auto inductionVar =
AxisInfo(knownContiguity, knownDivisibility, knownConstancy);
AxisInfo(std::move(knownContiguity), std::move(knownDivisibility),
std::move(knownConstancy));
(void)argLattices[0]->join(inductionVar);
}

Expand Down Expand Up @@ -1180,7 +1191,8 @@ void AxisInfo::initPessimisticStateFromFunc(int argNumber, T funcOp,
}
}

return AxisInfo(knownContiguity, knownDivisibility, knownConstancy);
return AxisInfo(std::move(knownContiguity), std::move(knownDivisibility),
std::move(knownConstancy));
}

/*static*/ AxisInfo AxisInfo::join(const AxisInfo &lhs, const AxisInfo &rhs) {
Expand All @@ -1202,7 +1214,8 @@ void AxisInfo::initPessimisticStateFromFunc(int argNumber, T funcOp,
rhs.getConstantValue().has_value() &&
lhs.getConstantValue() == rhs.getConstantValue())
constantValue = lhs.getConstantValue();
return AxisInfo(contiguity, divisibility, constancy, constantValue);
return AxisInfo(std::move(contiguity), std::move(divisibility),
std::move(constancy), constantValue);
}

unsigned ModuleAxisInfoAnalysis::getPtrContiguity(Value ptr) {
Expand Down