[PATCH] D63306: Add a remarks-based code size diffing tool

[Jessica Paquette via Phabricator via llvm-commits]

paquette marked 3 inline comments as done.
paquette added a comment.

In D63306#1543020 <https://reviews.llvm.org/D63306#1543020>, @echristo wrote:

> Haven't done much of a review, one drive by, but in general I like the idea. Would it make sense to have a general "remarks in object files" tool rather than just a size one? (It can, of course, only do size at the beginning.)
>
> Thoughts?


Hmmm. That sounds similar to a more detailed version of opt-diff? (With added support for object files)

(I suppose that basically every pass can impact code size though, so I could see basically every remark being correlated with size.)

Do you have anything in particular in mind for what a more general tool would look like?



================
Comment at: llvm/tools/sizediff/CMakeLists.txt:12
+if(LLVM_INSTALL_BINUTILS_SYMLINKS)
+  add_llvm_tool_symlink(sizeinfo sizediff)
+endif()
----------------
thegameg wrote:
> Any reason why both `sizeinfo` and `sizediff` are not prefixed with `llvm-`?
`sizeinfo` is a typo and shouldn't even be there, whoops.

`sizediff` is the best name I could think of, and I am entirely open to changing the name of this.

I think that `llvm-` is usually used to differentiate from existing UNIX/GNU tools (e.g. nm, size, etc.), but if people prefer the prefix, I don't mind at all.


================
Comment at: llvm/tools/sizediff/sizediff.cpp:53
+  // new file.
+  int Count[2] = {0, 0};
+
----------------
JDevlieghere wrote:
> I would use two integers with descriptive names, unless you expect to extend this in the future? You could keep `collectRemarks` generic and pass an enum instead of the index as the second argument, and have a setter with a switch.
I think descriptive names is a good idea.

I have to think about making `collectRemarks` generic. I'll play with teaching it to recognize some other types of remarks and see what it looks like.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63306/new/

https://reviews.llvm.org/D63306