This is an archive of the discontinued LLVM Phabricator instance.

Differential D116249

Demo fix for performance-move-const-arg (WIP)
ClosedPublic

Authored by mehdi_amini on Dec 23 2021, 2:22 PM.

Details

Reviewers
rriddle
Mogball
nicolasvasilache
Commits
rGee1fcb2fb684: Apply clang-tidy fixes for performance-move-const-arg to MLIR (NFC)

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 23 2021, 2:22 PM
Herald added subscribers: sdasgup3, wenzhicui, wrengr and 19 others. · View Herald TranscriptDec 23 2021, 2:22 PM
mehdi_amini requested review of this revision.Dec 23 2021, 2:22 PM
Herald added a reviewer: nicolasvasilache. · View Herald TranscriptDec 23 2021, 2:22 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
Harbormaster completed remote builds in B140563: Diff 396098.Dec 23 2021, 2:41 PM
Mogball accepted this revision.Dec 23 2021, 3:33 PM
This revision is now accepted and ready to land.Dec 23 2021, 3:33 PM
grandinj added a subscriber: grandinj.Dec 24 2021, 4:20 AM
grandinj added inline comments.
mlir/lib/Dialect/DLTI/DLTI.cpp
206

in this case, it is more likely that the checker has found a bug, where the loop variable should not be const

mehdi_amini added inline comments.Dec 24 2021, 4:35 PM
mlir/lib/Dialect/DLTI/DLTI.cpp
206

Thanks for the hint!
I'll fix this manually.

rriddle accepted this revision.Jan 1 2022, 3:34 AM

LGTM + the bug that Noel pointed out.

mehdi_amini abandoned this revision.Jan 1 2022, 5:16 PM
mehdi_amini reclaimed this revision.
This revision is now accepted and ready to land.Jan 1 2022, 5:16 PM
Closed by commit rGee1fcb2fb684: Apply clang-tidy fixes for performance-move-const-arg to MLIR (NFC) (authored by mehdi_amini). · Explain WhyJan 1 2022, 5:17 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added a commit: rGee1fcb2fb684: Apply clang-tidy fixes for performance-move-const-arg to MLIR (NFC).

Revision Contents

PathSize
mlir/
lib/
Dialect/
DLTI/
2 lines
test/
lib/
Dialect/
Linalg/
3 lines

Diff 396098

mlir/lib/Dialect/DLTI/DLTI.cpp

Show First 20 Lines • Show All 197 Lines • ▼ Show 20 LinescombineOneSpec(DataLayoutSpecInterface spec,
DenseMap<TypeID, DataLayoutEntryList> newEntriesForType; DenseMap<TypeID, DataLayoutEntryList> newEntriesForType;
DenseMap<StringAttr, DataLayoutEntryInterface> newEntriesForID; DenseMap<StringAttr, DataLayoutEntryInterface> newEntriesForID;
spec.bucketEntriesByType(newEntriesForType, newEntriesForID); spec.bucketEntriesByType(newEntriesForType, newEntriesForID);
// Try overwriting the old entries with the new ones. // Try overwriting the old entries with the new ones.
for (const auto &kvp : newEntriesForType) { for (const auto &kvp : newEntriesForType) {
if (!entriesForType.count(kvp.first)) { if (!entriesForType.count(kvp.first)) {
entriesForType[kvp.first] = std::move(kvp.second); entriesForType[kvp.first] = kvp.second;
grandinjUnsubmitted
Not Done
ReplyInline Actions

in this case, it is more likely that the checker has found a bug, where the loop variable should not be const

grandinj: in this case, it is more likely that the checker has found a bug, where the loop variable…
mehdi_aminiAuthorUnsubmitted
Done
ReplyInline Actions

Thanks for the hint!
I'll fix this manually.

mehdi_amini: Thanks for the hint! I'll fix this manually.
continue; continue;
} }
Type typeSample = kvp.second.front().getKey().get<Type>(); Type typeSample = kvp.second.front().getKey().get<Type>();
assert(&typeSample.getDialect() != assert(&typeSample.getDialect() !=
typeSample.getContext()->getLoadedDialect<BuiltinDialect>() && typeSample.getContext()->getLoadedDialect<BuiltinDialect>() &&
"unexpected data layout entry for built-in type"); "unexpected data layout entry for built-in type");
▲ Show 20 Lines • Show All 168 Lines • Show Last 20 Lines

mlir/test/lib/Dialect/Linalg/TestLinalgTransforms.cpp

Show First 20 Lines • Show All 522 Lines • ▼ Show 20 Lines vector::populateVectorTransferPermutationMapLoweringPatterns(
canonicalizationPatterns); canonicalizationPatterns);
vector::populateVectorReductionToContractPatterns(canonicalizationPatterns); vector::populateVectorReductionToContractPatterns(canonicalizationPatterns);
stage1Patterns.push_back(std::move(canonicalizationPatterns)); stage1Patterns.push_back(std::move(canonicalizationPatterns));
} }
SmallVector<FrozenRewritePatternSet, 4> frozenStage1Patterns; SmallVector<FrozenRewritePatternSet, 4> frozenStage1Patterns;
llvm::move(stage1Patterns, std::back_inserter(frozenStage1Patterns)); llvm::move(stage1Patterns, std::back_inserter(frozenStage1Patterns));
FrozenRewritePatternSet stage2Patterns = FrozenRewritePatternSet stage2Patterns =
getLinalgTilingCanonicalizationPatterns(ctx); getLinalgTilingCanonicalizationPatterns(ctx);
(void)applyStagedPatterns(funcOp, frozenStage1Patterns, (void)applyStagedPatterns(funcOp, frozenStage1Patterns, stage2Patterns);
std::move(stage2Patterns));
} }
static void applyVectorTransferForwardingPatterns(FuncOp funcOp) { static void applyVectorTransferForwardingPatterns(FuncOp funcOp) {
RewritePatternSet forwardPattern(funcOp.getContext()); RewritePatternSet forwardPattern(funcOp.getContext());
forwardPattern.add<LinalgCopyVTRForwardingPattern>(funcOp.getContext()); forwardPattern.add<LinalgCopyVTRForwardingPattern>(funcOp.getContext());
forwardPattern.add<LinalgCopyVTWForwardingPattern>(funcOp.getContext()); forwardPattern.add<LinalgCopyVTWForwardingPattern>(funcOp.getContext());
(void)applyPatternsAndFoldGreedily(funcOp, std::move(forwardPattern)); (void)applyPatternsAndFoldGreedily(funcOp, std::move(forwardPattern));
} }
▲ Show 20 Lines • Show All 187 Lines • Show Last 20 Lines