[CGData] Refactor Global Merge Functions #115750

kyulee-com · 2024-11-11T18:12:14Z

This is a follow-up PR to refactor the initial global merge function pass implemented in #112671.

It first collects stable functions relevant to the current module and iterates over those only, instead of iterating through all stable functions in the stable function map.

This is a patch for https://discourse.llvm.org/t/rfc-global-function-merging/82608.

nocchijiang

This refactoring makes much more sense to me than the previous implementation.

nocchijiang · 2024-11-12T01:23:54Z

llvm/lib/CodeGen/GlobalMergeFunctions.cpp

-    // Parameter locations based on the unique hash sequences
-    // across the candidates.
+  // Collect stable functions related to the current module.
+  DenseMap<stable_hash, SmallVector<Function *>> HashToFuncs;


Suggested change

DenseMap<stable_hash, SmallVector<Function *>> HashToFuncs;

DenseMap<stable_hash, SmallVector<std::pair<Function *, FunctionHashInfo>>> HashToFuncs;

To save the cost of FuncToFI. Or maybe we could put Function * in FunctionHashInfo?

nocchijiang · 2024-11-12T01:25:48Z

llvm/lib/CodeGen/GlobalMergeFunctions.cpp

-      }
+    // Iterate functions with the same hash.
+    for (auto &F : Funcs) {
+      auto &SFS = Maps.at(Hash);


Hopefully this will be hoisted out of the for loop by the compiler.

nocchijiang · 2024-11-12T01:36:27Z

llvm/lib/CodeGen/GlobalMergeFunctions.cpp

+      // Check if the function is compatible with any stable function
+      // in terms of the number of instructions and ignored operands.
+      assert(!SFS.empty());
+      auto &RFS = SFS[0];


I seem to understand the potential size increase introduced by the refactoring: is it possible that stable function entries with the same hash could have different numbers of / incompatible operands? If that is the case, I believe we can assume that the probability of hash collisions should be very low, otherwise we might actually need to improve the hashing algorithm.

Or maybe we can move the "lightweight" checks back into the nested loop below?

If the codegen data is not stale —meaning there has been no source change in the first or second pass— the size regression is less of a concern. My main worry was about the stability of optimization when the codegen data becomes outdated.

nocchijiang · 2024-11-12T06:51:12Z

Hit an assertion in ignoreOp when testing the refactored code.

Assertion failed: (OpIdx < I->getNumOperands() && "Invalid operand index"), function ignoreOp, file GlobalMergeFunctions.cpp, line 129.
Stop reason: hit program assert
expr I->dump()
  %6 = tail call ptr @objc_retain(ptr %5), !dbg !576

p I->getNumOperands()

(unsigned int) 2
p OpIdx

(unsigned int) 4

kyulee-com · 2024-11-12T14:31:30Z

Hit an assertion in ignoreOp when testing the refactored code.

Assertion failed: (OpIdx < I->getNumOperands() && "Invalid operand index"), function ignoreOp, file GlobalMergeFunctions.cpp, line 129.
Stop reason: hit program assert
expr I->dump()
  %6 = tail call ptr @objc_retain(ptr %5), !dbg !576

p I->getNumOperands()

(unsigned int) 2
p OpIdx

(unsigned int) 4

Thank you for testing and identifying this bug!
Since we also use this function to verify any function that matches a hash, it should not assert.

ellishg · 2024-11-12T18:12:48Z

Hit an assertion in ignoreOp when testing the refactored code.

Assertion failed: (OpIdx < I->getNumOperands() && "Invalid operand index"), function ignoreOp, file GlobalMergeFunctions.cpp, line 129.
Stop reason: hit program assert
expr I->dump()
  %6 = tail call ptr @objc_retain(ptr %5), !dbg !576

p I->getNumOperands()

(unsigned int) 2
p OpIdx

(unsigned int) 4

Do we know why OpIdx is 4 here? This is confusing to me because it looks like there is only one argument, %5.

kyulee-com · 2024-11-12T18:30:40Z

Do we know why OpIdx is 4 here? This is confusing to me because it looks like there is only one argument, %5.

The ignoreOp function was initially designed for use with llvm::StructuralHashWithDifferences, where it iterates over operands within the same instruction. In this context, OpIdx is always within the valid range for the specified instruction.
However, we now also utilize this function to determine if a particular operand can be ignored in certain instructions during this merge operation, as matched in the stable function summary— see hasValidSharedConst() for its use. So, there may be cases where an out-of-range index is passed from a different instruction context (although the entire function hash is matched). In this case, we should simply return false, as the target operand is not an interesting operand (that can be ignored/parameterized for merging).

kyulee-com · 2024-11-13T15:29:57Z

@nocchijiang The new approach seems to be functioning well and is similar in size to the previous method.
I suspect that the no-LTO case might still encounter some slowdown, as each CU needs to read the entire CGData regardless. Currently, the CGData used for this merging process does not utilize names, which means we could potentially eliminate strings or make them optional. Alternatively, we could restructure the indexed CGData to allow for reading only the relevant hash entries on demand. I'd like to leave these options open for now, and if you can continue to improve it, that would be excellent.

nocchijiang · 2024-11-14T00:08:58Z

I suspect that the no-LTO case might still encounter some slowdown, as each CU needs to read the entire CGData regardless.

I can confirm that the performance have been improved significantly from my testing on no-LTO projects that the slowdown is acceptable now. Before applying the PR it was about 50% slowdown, now it is ~5%.

Alternatively, we could restructure the indexed CGData to allow for reading only the relevant hash entries on demand.

Besides only consuming the matched stable entries like what this PR does, this is exactly what I planned to do to reduce the memory footprint of the deserialized CGData. I would like to discuss the detail in the RFC thread with you to make sure that we are on the same page before coding it.

kyulee-com · 2024-11-14T01:16:50Z

I can confirm that the performance have been improved significantly from my testing on no-LTO projects that the slowdown is acceptable now. Before applying the PR it was about 50% slowdown, now it is ~5%.

That's great to hear!
Since these PRs appear to be functioning, is it okay to merge them for now while we continue to discuss further improvements? Or do you have more comments to be addressed?

llvm-ci · 2024-11-14T05:18:48Z

LLVM Buildbot has detected a new failure on builder lldb-x86_64-debian running on lldb-x86_64-debian while building llvm at step 4 "build".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/10436