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.

These are updates to try to fix the build problem of the llvm-sim executable not being found for tests on Windows and some Linux builds. Unfortunately, I do not have the errors anymore. Is there something else that will need added to add llvm-sim as a tool? The changes I've made reflect what I've found has been done for other tools.

I think if the errors on Windows and Linux are gone, it's fine to recommit.

There's a bug with one of the Windows buildbots where the error message doesn't match (even though it looks like it does), but I haven't quite been able to figure out why it's failing yet. I think it has something do to with the fact that it's reporting an error, and that there is a FileCheck prefix.

https://lab.llvm.org/buildbot#builders/123/builds/3498

There's a bug with one of the Windows buildbots where the error message doesn't match (even though it looks like it does), but I haven't quite been able to figure out why it's failing yet. I think it has something do to with the fact that it's reporting an error, and that there is a FileCheck prefix.

https://lab.llvm.org/buildbot#builders/123/builds/3498

Visual Studios Standard library starts their errors in lower case, which do not match the hardcoded values you used in your tests. Take a look at the lit feature added here https://reviews.llvm.org/rGc52fe0b02172e707aa2ba38cd2e01a1fc70dd0da, you should be able to use it to expand to the correct platform specific error message

Visual Studios Standard library starts their errors in lower case, which do not match the hardcoded values you used in your tests. Take a look at the lit feature added here https://reviews.llvm.org/rGc52fe0b02172e707aa2ba38cd2e01a1fc70dd0da, you should be able to use it to expand to the correct platform specific error message

Ah I see, that only applies to the second part of the error right? The first part (Could not open file) is LLVM generated I think.

Visual Studios Standard library starts their errors in lower case, which do not match the hardcoded values you used in your tests. Take a look at the lit feature added here https://reviews.llvm.org/rGc52fe0b02172e707aa2ba38cd2e01a1fc70dd0da, you should be able to use it to expand to the correct platform specific error message

Ah I see, that only applies to the second part of the error right? The first part (Could not open file) is LLVM generated I think.

Looks like it to me yes

Updating message for the failure case where the file does not exist to be platform agnostic.

I think with the generalization this should be good to try to land again?

This revision was landed with ongoing or failed builds.Fri, Jun 11, 1:04 PM

This seems to break tests everywhere, eg http://45.33.8.238/linux/48721/step_12.txt

Please take a look and revert for now if it takes a while to fix.

This seems to break tests everywhere, eg http://45.33.8.238/linux/48721/step_12.txt

Please take a look and revert for now if it takes a while to fix.

Is this build on the buildbot site? Or do you have any insights how to fix it? It seems related to the gn build system, which I don't have any experience with.

Matt added a subscriber: Matt.Sat, Jun 12, 10:03 AM