This is an archive of the discontinued LLVM Phabricator instance.

Add a tool for diffing instruction count + stack size remarks
ClosedPublic

Authored by paquette on Nov 1 2021, 11:02 AM.

Details

Summary

This is a tool which can handle bitstream and YAML remarks. The idea here is to provide more insight into which functions changed in a benchmark when testing compiler changes.

E.g. "foo got 20% bigger, so maybe we should look more closely at that."

To use the tool, you can use...

$ llvm-remark-size-diff remarks_file_a remarks_file_b --parser=yaml|bitstream

... on two remarks files containing at least instruction count remarks. This will output some data on instruction count change and also other relevant information such as stack size change from remarks_file_a to remarks_file_b.

This is a bit of a WIP so I'm happy to change the format etc. Ultimately I think it'd be best to have some JSON output which could be consumed by another tool. But some base-level, greppable output is very handy to have anyway.

The format I'm proposing here is

<which files?> <increase/decrease in instruction count?> <function name> <inst count change> <stack B change>

Where the files and increase/decrease are indicated like below:

  • <which files> is one of ++ (file B), -- (file A), == (both)
  • <increase/decrease in instruction count?> is one of > (increase) or < (decrease)

This makes it easy to grep for things like "which functions appeared in A but did not appear in B?" Or "what are all the instruction count decreases?"

Diff Detail

Event Timeline

paquette created this revision.Nov 1 2021, 11:02 AM
paquette requested review of this revision.Nov 1 2021, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 11:02 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
keith added a subscriber: keith.Nov 1 2021, 11:13 AM

This is great, thanks! In general everything looks pretty good to me. I'll take a second look while others get some time to comment too!

llvm/tools/llvm-size-diff/SizeDiff.cpp
299

you can probably use remarks::Format::YAML and remarks::Format::Bitstream directly here

I've not looked into this patch at all, but my Herald rule got triggered, as I'm subscribed to llvm-size changes. That made me think it would be good if we could pick a different name that isn't going to have a prefix clash with other tools? This will help with some shells auto-completion, if nothing else! I don't have any concrete suggestions, but perhaps llvm-remarks-diff or something like that?

llvm/test/tools/llvm-size-diff/no-instruction-count-remarks.test
4

Nit: no new line at EOF

I've not looked into this patch at all, but my Herald rule got triggered, as I'm subscribed to llvm-size changes. That made me think it would be good if we could pick a different name that isn't going to have a prefix clash with other tools? This will help with some shells auto-completion, if nothing else! I don't have any concrete suggestions, but perhaps llvm-remarks-diff or something like that?

Sure.

Not sure if remarks diff works either, since there's already opt-diff etc, which have a slightly different purpose than this tool. I'll think of something though.

asb added a subscriber: asb.Nov 4 2021, 5:53 AM
paquette updated this revision to Diff 387344.Nov 15 2021, 11:43 AM

Address review comment + change name to avoid terminal completion issues with llvm-size

(I wanted to keep the "size" part in the name because I want to focus this tool around instruction count)

paquette edited the summary of this revision. (Show Details)Nov 15 2021, 11:44 AM

Address review comment + change name to avoid terminal completion issues with llvm-size

(I wanted to keep the "size" part in the name because I want to focus this tool around instruction count)

Thanks. Name change seems reasonable to me.

I noticed the pre-merge checks are failing. It seems to not like the new executable name, or there's something else missing (do you need to add this tool to the list of subsitutions lit performs?).

paquette updated this revision to Diff 387705.Nov 16 2021, 11:06 AM

Add the tool to the lit tool substitutions. (Not sure if this is the correct way to do this, let's see!)

phosek added a subscriber: phosek.Nov 16 2021, 11:17 PM

Add the tool to the lit tool substitutions. (Not sure if this is the correct way to do this, let's see!)

I think what you've done is right, but apparently it's not the issue.

I just noticed that all the failures are on debian machines, but I think there's also Windows machines that run the pre-merge checks. This could be a red-herring though. Sorry, I'm out of further ideas.

Add the tool to the lit tool substitutions. (Not sure if this is the correct way to do this, let's see!)

I think what you've done is right, but apparently it's not the issue.

I just noticed that all the failures are on debian machines, but I think there's also Windows machines that run the pre-merge checks. This could be a red-herring though. Sorry, I'm out of further ideas.

Ignore what I just said there about Windows. For some reason, some parts aren't showing up, but the Windows ones are failing in the same way. On further inspection of the build log, the reason that the tool isn't being found is because for whatever reason, it isn't being built, based on my reading of the log at least. Again, I'm not sure why. I'd blame something in cmake, but I don't know why you wouldn't see the failure locally in this case.

Maybe the pre-merge check is incremental, and it needs a ninja clean? At least on my machine, the tool required that to build.

paquette updated this revision to Diff 388009.Nov 17 2021, 11:58 AM

Adding the tool to llvm/test/CMakeLists.txt to see if that helps (as suggested by @fhahn offline)

Matt added a subscriber: Matt.Nov 17 2021, 11:59 AM

Great to see this coming along :)

Do you think there is a way to generalize this to diff arbitrary remarks or are you envisioning having separate tools for diffing separate remarks? (I didn't read the full discussion, apologies if this was already covered)

I was thinking about keeping it focused on instruction count as the most important metric, with a greppable output to start.

I think it could be generalized though. I think that a really important next step for this is to have a machine-consumable output mode (e.g. JSON) which could have custom views built around it.

Theoretically, this greppable output could be changed into a custom view which is built from the, say, JSON.

I guess that once we have a machine-consumable output, there's really no reason to say that instruction count would be the most important thing.

mehdi_amini added inline comments.
llvm/tools/llvm-remark-size-diff/RemarkSizeDiff.cpp
239 ↗(On Diff #388009)

You need to use long long for this variable to match the signature of getAsSignedInteger apparently

paquette updated this revision to Diff 388352.Nov 18 2021, 4:38 PM

Use long long explicitly in getIntValFromKey

I think it could be generalized though. I think that a really important next step for this is to have a machine-consumable output mode (e.g. JSON) which could have custom views built around it.

Maybe having python bindings that work with bitstream on top of the C API can make this even easier? Going back to a textual format seems wasteful and won't scale when a lot more remarks are enabled.

Hmm yeah I think you're right.

thegameg accepted this revision.Feb 2 2022, 2:13 PM

LGTM on my side. If this needs to grow into a more generic tool I think we can go with the python bindings and other things at that time.

This revision is now accepted and ready to land.Feb 2 2022, 2:13 PM
This revision was automatically updated to reflect the committed changes.

if this isn't the case, it would be nice to add this to the release notes :)