[PATCH] D105284: Greedy set cover implementation of `Merger::Merge`

Kostya Serebryany via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 12 12:43:52 PDT 2021


kcc added a comment.

Thanks for the change! 
Indeed, the current single-pass merge is far from perfect, and it's nice to see your numbers.

Some high-level notes:

- not sure about the flag syntax (-merge=2), I'd prefer something more descriptive. Maybe add an extra flag -set_cover_merge=1?
- The current single-pass algorithm is also greedy (just less so :)), so maybe use a different name to distinguish the new algorithm
- please try to follow the coding style e.g. with respect to {} around single-statement blocks.
- please add a unit test and a .lit test

I'll let Matt make another round of review.


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

https://reviews.llvm.org/D105284



More information about the llvm-commits mailing list