Page MenuHomePhabricator

[IRSim] Adding basic implementation of llvm-sim.
ClosedPublic

Authored by AndrewLitteken on Sep 1 2020, 1:21 PM.

Details

Summary

This is a similarity visualization tool that accepts a Module and passes it to the IRSimilarityIdentifier. The resulting SimilarityGroups are output in a JSON file.

Tests are found in test/tools/llvm-sim and check for the file not found, a bad module, and that the JSON is created correctly.

Example of what can be built on top of the JSON output:

Diff Detail

Event Timeline

AndrewLitteken created this revision.Sep 1 2020, 1:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 1:21 PM
AndrewLitteken edited the summary of this revision. (Show Details)Sep 1 2020, 2:27 PM
AndrewLitteken edited the summary of this revision. (Show Details)
AndrewLitteken added a reviewer: paquette.

Updating for clang-format.

jroelofs added inline comments.
llvm/tools/llvm-sim/FindSimilarities.cpp
50 ↗(On Diff #289470)

A useful pattern for tools like this (and I think this might forbid?) is to support reading from stdin / writing to stdout when the filename is -. That's also useful for the tests, because it lets you eliminate the cat %t RUN lines.

llvm/tools/llvm-sim/FindSimilarities.h
27 ↗(On Diff #289470)

StringRef?

58 ↗(On Diff #289470)

StringRef?

llvm/tools/llvm-sim/JSONExporter.h
44 ↗(On Diff #289470)

A free function would probably be fine. The class doesn't seem to be buying you much here.

llvm/tools/llvm-sim/llvm-sim.cpp
37

I think it would clean up the error handling a bit if the file handling & bitcode reading all happened here, and then have this transfer ownership of the constructed module into the tool object.

That said, the tool object isn't buying you much either. I'd probably end up folding most/all of it into this file, with helper functions as needed.

Reorganization, and implication of string handling.

This revision is now accepted and ready to land.Sep 15 2020, 8:35 AM
vsk added a subscriber: vsk.Sep 16 2020, 9:48 AM
vsk added inline comments.
llvm/tools/llvm-sim/llvm-sim.cpp
134

If it's not necessary/desirable to number meta-instructions (e.g. debug info intrinsics), BB.instructionsWithoutDebug() might be a better fit here.

vsk added inline comments.Sep 16 2020, 9:50 AM
llvm/tools/llvm-sim/llvm-sim.cpp
135

Very minor nit -- any reason not to prefer a shorter way to write this, maybe 'map[&I] = Num++' or 'map.emplace(&I, Num++)' if it's necessary to avoid default-constructing a DenseMapPair?

AndrewLitteken added inline comments.Sep 17 2020, 1:41 PM
llvm/tools/llvm-sim/llvm-sim.cpp
134

I agree, that would probably be better here.

135

I don't think so. I just didn't realize those shortcuts existed.

Excluding debug instructions, and updating how we add to the DenseMap.

vsk added a comment.Sep 18 2020, 1:58 PM

(This is really nice work! FTR, my comments were drive-bys, I didn't intend for them to be blocking.)

This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.
llvm/test/tools/llvm-sim/fail-cases.test
3

For FileCheck, double-dash --check-prefix is preferred

llvm/test/tools/llvm-sim/single-sim-file.test
3

FileCheck %s < %t

Fixing CMakeLists.txt to accurately reflect what needs to be linked, and flags and redirection for the tests themselves.

Updating LLVMBuild.txt as well.