[llvm] Port Swift's merge function pass to llvm: merging functions that differ in constants (PR #68235)

Aiden Grossman via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 18 11:58:36 PDT 2023


boomanaiden154 wrote:

> Yes we should try to reduce code duplication. By looking at FunctionHashIgnoringConst, there are a few issues: FunctionHashIgnoringConst currently inherits from FunctionComparatorIgnoringConst. --> we call beginCompare, and cmpConstants from FunctionComparator --> also use GlobalNumberState from FunctionComparator We also have two optional parameters to save extra information: std::map<int, Instruction *> *IdxToInst = nullptr, std::map<std::pair<int, int>, uint64_t> *IdxToConstHash = nullptr --> I guess we can keep them as optional in StructuralHash. How many setting should we support in StructuralHash? It currently supports default and DetailedHash. Does it mean we will have default, DetailedHash, and IgnoringConst?

We should be able to just add the instruction and constant indexes to `StructuralHash`. All of the new additions should be gated behind the `DetailedHash` flag, and then I'd suggest adding a new flag like `IgnoringConst` that ignores constants when it's set to true.

`beginCompare` only clears the maps, so I don't think that should be a major issue. `GlobalNumberState` is something that is currently missing, so refactoring/moving code might be necessary there depending upon how others think everything should be implemented.

> Yes, I will split the patch into hashing changes and non-hashing changes.

Great! Splitting the hashing changes into even smaller portions (splitting them like I did above, or however you choose) would probably be even more ideal as they're relatively independent changes and that will probably make review/getting them in easier rather than trying to get a single monolithic change in.

https://github.com/llvm/llvm-project/pull/68235


More information about the llvm-commits mailing list