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

Jump to bottom

[FIRRTL] Dedup: speed up handling of instances #7815

Merged
merged 1 commit into from
Nov 14, 2024
Merged

[FIRRTL] Dedup: speed up handling of instances #7815

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 25 additions & 27 deletions lib/Dialect/FIRRTL/Transforms/Dedup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseMapInfo.h"
#include "llvm/ADT/DepthFirstIterator.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/TypeSwitch.h"
Expand Down Expand Up @@ -89,9 +90,14 @@ struct ModuleInfo {
// SHA256 hash.
std::array<uint8_t, 32> structuralHash;
// Module names referred by instance op in the module.
mlir::ArrayAttr referredModuleNames;
std::vector<StringAttr> referredModuleNames;
Copy link
Contributor

Choose a reason for hiding this comment

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

SmallVector should be fine for these.

};

static bool operator==(const ModuleInfo &lhs, const ModuleInfo &rhs) {
return lhs.structuralHash == rhs.structuralHash &&
lhs.referredModuleNames == rhs.referredModuleNames;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance this vector comparison rather expensive than interning them as ArrayAttr? Not sure how common it is but if there are modules that has same hash and instances within them refer to different modules, it could be possible that this comparison occurs multiple times.

Copy link
Member Author

@youngar youngar 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.

Yeah, its possible it does the comparison multiple times. I was thinking that the same comparison problem could happen when creating the ArrayAttr by interning the vector in a DenseSet, so its probably similar in the worst case of both. I just noticed that StorageUniquer stores a key's hash as part of the key, which seems like it could help reduce expensive comparisons 🤔

}

/// Unique identifier for a value. All value sources are numbered by apperance,
/// and values are identified using this numbering (`index`) and an `offset`.
/// For BlockArgument's, this is the argument number.
Expand Down Expand Up @@ -146,11 +152,9 @@ struct StructuralHasher {
explicit StructuralHasher(const StructuralHasherSharedConstants &constants)
: constants(constants){};

std::pair<std::array<uint8_t, 32>, SmallVector<StringAttr>>
getHashAndModuleNames(FModuleLike module) {
ModuleInfo getModuleInfo(FModuleLike module) {
update(&(*module));
auto hash = sha.final();
return {hash, referredModuleNames};
return {sha.final(), std::move(referredModuleNames)};
}

private:
Expand Down Expand Up @@ -359,7 +363,7 @@ struct StructuralHasher {
DenseMap<StringAttr, SymbolTarget> innerSymTargets;

// This keeps track of module names in the order of the appearance.
SmallVector<mlir::StringAttr> referredModuleNames;
std::vector<StringAttr> referredModuleNames;
Copy link
Contributor

Choose a reason for hiding this comment

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

SmallVector should still be fine for these.


// String constants.
const StructuralHasherSharedConstants &constants;
Expand Down Expand Up @@ -1595,13 +1599,13 @@ struct DenseMapInfo<ModuleInfo> {
static inline ModuleInfo getEmptyKey() {
std::array<uint8_t, 32> key;
std::fill(key.begin(), key.end(), ~0);
return {key, DenseMapInfo<mlir::ArrayAttr>::getEmptyKey()};
return {key, {}};
}

static inline ModuleInfo getTombstoneKey() {
std::array<uint8_t, 32> key;
std::fill(key.begin(), key.end(), ~0 - 1);
return {key, DenseMapInfo<mlir::ArrayAttr>::getTombstoneKey()};
return {key, {}};
}

static unsigned getHashValue(const ModuleInfo &val) {
Expand All @@ -1611,12 +1615,13 @@ struct DenseMapInfo<ModuleInfo> {
std::memcpy(&hash, val.structuralHash.data(), sizeof(unsigned));

// Combine module names.
return llvm::hash_combine(hash, val.referredModuleNames);
return llvm::hash_combine(
hash, llvm::hash_combine_range(val.referredModuleNames.begin(),
val.referredModuleNames.end()));
}

static bool isEqual(const ModuleInfo &lhs, const ModuleInfo &rhs) {
return lhs.structuralHash == rhs.structuralHash &&
lhs.referredModuleNames == rhs.referredModuleNames;
return lhs == rhs;
}
};
} // namespace llvm
Expand Down Expand Up @@ -1659,9 +1664,7 @@ class DedupPass : public circt::firrtl::impl::DedupBase<DedupPass> {
return cast<FModuleLike>(*node->getModule());
}));

SmallVector<std::optional<
std::pair<std::array<uint8_t, 32>, SmallVector<StringAttr>>>>
hashesAndModuleNames(modules.size());
SmallVector<std::optional<ModuleInfo>> moduleInfos(modules.size());
StructuralHasherSharedConstants hasherConstants(&getContext());

// Attribute name used to store dedup_group for this pass.
Expand Down Expand Up @@ -1708,7 +1711,7 @@ class DedupPass : public circt::firrtl::impl::DedupBase<DedupPass> {

StructuralHasher hasher(hasherConstants);
// Calculate the hash of the module and referred module names.
hashesAndModuleNames[idx] = hasher.getHashAndModuleNames(module);
moduleInfos[idx] = hasher.getModuleInfo(module);
return success();
});

Expand All @@ -1717,9 +1720,9 @@ class DedupPass : public circt::firrtl::impl::DedupBase<DedupPass> {

for (auto [i, module] : llvm::enumerate(modules)) {
auto moduleName = module.getModuleNameAttr();
auto &hashAndModuleNamesOpt = hashesAndModuleNames[i];
auto &maybeModuleInfo = moduleInfos[i];
// If the hash was not calculated, we need to skip it.
if (!hashAndModuleNamesOpt) {
if (!maybeModuleInfo) {
// We record it in the dedup map to help detect errors when the user
// marks the module as both NoDedup and MustDedup. We do not record this
// module in the hasher to make sure no other module dedups "into" this
Expand All @@ -1728,16 +1731,11 @@ class DedupPass : public circt::firrtl::impl::DedupBase<DedupPass> {
continue;
}

// Replace module names referred in the module with new names.
SmallVector<mlir::Attribute> names;
for (auto oldModuleName : hashAndModuleNamesOpt->second) {
auto newModuleName = dedupMap[oldModuleName];
names.push_back(newModuleName);
}
auto &moduleInfo = maybeModuleInfo.value();

// Create a module info to use it as a key.
ModuleInfo moduleInfo{hashAndModuleNamesOpt->first,
mlir::ArrayAttr::get(module.getContext(), names)};
// Replace module names referred in the module with new names.
for (auto &referredModule : moduleInfo.referredModuleNames)
referredModule = dedupMap[referredModule];

// Check if there a module with the same hash.
auto it = moduleInfoToModule.find(moduleInfo);
Expand All @@ -1755,7 +1753,7 @@ class DedupPass : public circt::firrtl::impl::DedupBase<DedupPass> {
// Add the module to a new dedup group.
dedupMap[moduleName] = moduleName;
// Record the module info.
moduleInfoToModule[moduleInfo] = module;
moduleInfoToModule[std::move(moduleInfo)] = module;
}

// This part verifies that all modules marked by "MustDedup" have been
Expand Down
Loading