This is an archive of the discontinued LLVM Phabricator instance.

Make the FunctionComparator of the MergeFunctions pass a stand-alone utility in a separate file
ClosedPublic

Authored by eeckstein on Oct 7 2016, 4:57 PM.

Details

Summary

This is pure refactoring. NFC.

Motivation: in the swift compiler we have a pass, called SwiftMergeFunctions, which is a modified version of llvm's MergeFunctions:
https://github.com/apple/swift/blob/master/lib/LLVMPasses/LLVMMergeFunctions.cpp

The purpose of SwiftMergeFunctions is to merge _similar_ functions which only differ by some constants. It's used to merge specialized generic functions.
Some background info: it's important that SwiftMergeFunctions runs _after_ llvm's MergeFunctions, because we want to merge similar functions only after all equivalent functions are already merged.

SwiftMergeFunctions shares most of the FunctionComparator code with the llvm pass. Currently this code is just copied.

This change moves the FunctionComparator (together with the GlobalNumberState utility) in to a separate file so that it can be used by both passes. The SwiftMergeFunctions pass derives its own comparator from FunctionComparator. This is how it looks like: https://github.com/eeckstein/swift/blob/merge-func-refactor/lib/LLVMPasses/LLVMMergeFunctions.cpp

Details of the change:
*) The big part is just moving code out of MergeFunctions.cpp into FunctionComparator.h/cpp
*) Make FunctionComparator member functions protected (instead of private) so that the derived comparator in SwiftMergeFunctions can use them.

Following refactoring helps to share code between the base FunctionComparator class and the derived class:
*) Add a beginCompare() function
*) Move some basic function property comparisons into a separate function compareSignature()
*) Do the GEP comparison inside cmpOperations() which now has a new needToCmpOperands reference parameter

Diff Detail

Event Timeline

eeckstein updated this revision to Diff 73999.Oct 7 2016, 4:57 PM
eeckstein retitled this revision from to Make the FunctionComparator of the MergeFunctions pass a stand-alone utility in a separate file.
eeckstein updated this object.
eeckstein added reviewers: jfb, dyatkovskiy, nicholas.
eeckstein added a subscriber: llvm-commits.

This makes sense. I shared our own similar function merging pass (in D22051), which is more generic than the Swift one, and it could potentially also use this infrastructure. We could go a little further and pull out more of the stuff in MergeFunctions that they all share (e.g. thunk creation, block traversal) in the future.

lib/Transforms/Utils/FunctionComparator.cpp
476

Do you really need needToCmpOperands? You could just check in the caller whether isa<GetElementPtrInst>(L). Do you foresee any uses other than GEPs?

Might be worth having some unit tests if it's nicely abstracted as a reusable utility.

eeckstein added inline comments.Oct 10 2016, 3:29 PM
lib/Transforms/Utils/FunctionComparator.cpp
476

It was my intention that a caller of cmpOperations does not have to check for GEPs. In the SwiftMergeFunctions pass there is another caller and I didn't wanted to duplicate the code (which checks for GEPs). I thought it's a little bit safer this way.

Might be worth having some unit tests if it's nicely abstracted as a reusable utility.

Good point. I'll add some unittests

eeckstein updated this revision to Diff 74426.Oct 12 2016, 1:07 PM

I added a unit test for FunctionComparator. It mainly test the API of the class (I didn't want to replicate the test for functionality, which are covered by the MergeFunction pass tests).

Also, I made some functions private which are not covered by the API test and which I think are not important to be accessible - at this time, but it may change in future.

dberris added inline comments.
unittests/Transforms/Utils/FunctionComparator.cpp
21

nit: Don't think you need this space here.

109

Does this one test cover enough of the cases? I see one set of calls for the same function, without covering negative cases -- though I suspect since this is a refactoring that other tests ensure that this doing what is intended?

More concretely -- have you considered breaking up the big function API into smaller pieces that can be tested independently?

eeckstein added inline comments.Nov 8 2016, 7:37 AM
unittests/Transforms/Utils/FunctionComparator.cpp
109

The main purpose of this unit test is to check the public API of FunctionComparator. I added this unit test because currently this API is used in an external project (the swift compiler).

Testing the functionality is mainly done with the tests in test/Transforms/MergeFunc

dberris accepted this revision.Nov 8 2016, 8:55 PM
dberris added a reviewer: dberris.

From having a look at this, I don't see anything obviously wrong, and I suspect adding more tests later on would be fine too.

This revision is now accepted and ready to land.Nov 8 2016, 8:55 PM

Committed revision 286632.

Thanks for reviewing!

jfb closed this revision.May 7 2018, 9:59 PM

Looks like this was committed but not closed.