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.

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

[Analysis] Add OpCount Analysis #7644

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Conversation

TaoBi22
Copy link
Contributor

@TaoBi22 TaoBi22 commented Sep 27, 2024

Adds a very simple analysis pass to collect information about the kinds of operations (and operand counts) found in a hw.module so we can look at operation usage across big designs. Once this is upstream I'll add a pass to export this info as JSON.

Comment on lines +36 to +37
DenseMap<OperationName, size_t> opCounts;
DenseMap<OperationName, DenseMap<size_t, size_t>> operandCounts;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

opCounts could be calculated from operandCounts here, but it would require a lot of extra iterating over DenseMaps so went for the faster option since we probably don't want this to be slower than necessary if it's running on huge blocks of IR.

@dtzSiFive
Copy link
Contributor

Awesome, thanks for putting this together!

Looks like this is similar to upstream MLIR's print-op-stats pass -- not currently registered re:circt-opt but can be added, FWIW.

Have you seen this, have thoughts on how this compares?

At a glance, this seems to additionally bin by operand count, not just operation name...?

@TaoBi22
Copy link
Contributor Author

TaoBi22 commented Sep 27, 2024

Looks like this is similar to upstream MLIR's print-op-stats pass -- not currently registered re:circt-opt but can be added, FWIW.

Good point, I wasn't actually aware of that one! They definitely look similar - I think the big difference is that this is an analysis rather than just a pass. This might be handy as this is being upstreamed so we can get various pieces of data on how different operations are actually used in CIRCT (e.g. the spread of operand counts on variadic operations, hence the binning by operand count), and it would be interesting to be able to directly query the analysis as different transformations are done on the IR. Can drop the operation counting if this smells like duplication of functionality though.

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

It would be interesting if this could unify with upstream. That said, this is fine to go in as we need it for running some experiments. (If we completely throw this away in the future after it's served it's purpose, that's fine, too!)

include/circt/Analysis/OpCountAnalysis.h Outdated Show resolved Hide resolved
include/circt/Analysis/OpCountAnalysis.h Show resolved Hide resolved
lib/Analysis/OpCountAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/OpCountAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/OpCountAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/OpCountAnalysis.cpp Outdated Show resolved Hide resolved
Comment on lines +278 to +289
for (auto opName : opNames) {
llvm::errs() << opName << ": " << opCount.getOpCount(opName) << "\n";
auto operandMap = opCount.getOperandCountMap(opName);
// Sort for determinism again
llvm::SmallVector<size_t> keys;
for (auto pair : operandMap)
keys.push_back(pair.first);
llvm::sort(keys);
for (auto num : keys)
llvm::errs() << " with " << num << " operands: " << operandMap[num]
<< "\n";
}
Copy link
Member

Choose a reason for hiding this comment

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

I'll leave this format to you, particularly since you mentioned bridging this to JSON or something like that.

I tend to actually export this information directly as YAML (manually, not using the internal library). You may be able to go to JSON directly here if you want.

Regardless, I think the format should be less wordy eventually, e.g.:

operations:
  - name: builtin.module
    count: 1
  - name: comb.add
    count: 4
    operandCounts:
      - operands: 1
        count: 2
      - operands: 2
        count: 4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just put this in for testing so figured something readable in case of debugging failure wouldn't hurt. Happy to trim it down if preferred though.

Copy link
Member

@maerhart maerhart left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

include/circt/Analysis/OpCountAnalysis.h Outdated Show resolved Hide resolved
lib/Analysis/OpCountAnalysis.cpp Outdated Show resolved Hide resolved
lib/Analysis/OpCountAnalysis.cpp Outdated Show resolved Hide resolved
namespace {
struct TestOpCountAnalysisPass
: public PassWrapper<TestOpCountAnalysisPass, OperationPass<ModuleOp>> {
MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOpCountAnalysisPass)
Copy link
Member

Choose a reason for hiding this comment

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

If you plan to add a proper pass to print it as JSON, why do we even need a separate test pass? That proper pass could support two formats (a human-readable one and an easy-to-parse one) like the upstream variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I just put this in so we had something upstream quickly to start getting data while I did the JSON pass, but definitely agree it would make sense to have the JSON pass also do a human readable form.

@TaoBi22
Copy link
Contributor Author

TaoBi22 commented Oct 1, 2024

Merging this now, I'll get another PR in shortly to do JSON export (and move the readable export into that pass too). I'll make the format less wordy as requested by @seldridge too

@TaoBi22 TaoBi22 merged commit 8f2a791 into llvm:main Oct 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants