[CGData][GMF] Skip No Params #116548

kyulee-com · 2024-11-17T16:50:53Z

This update follows up on change #112671 and is mostly a NFC, with the following exceptions:

Introduced -global-merging-skip-no-params to bypass merging when no parameters are required.
Parameter count is now calculated based on the unique hash count.
Added -global-merging-inst-overhead to adjust the instruction overhead, reflecting the machine instruction size.
Costs and benefits are now computed using the double data type. Since the finalization process occurs offline, this should not significantly impact build time.
Moved a sorting operation outside of the loop.

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

kyulee-com · 2024-11-17T21:07:32Z

nocchijiang · 2024-11-18T00:47:23Z

llvm/lib/CGData/StableFunctionMap.cpp

+  SmallSet<stable_hash, 8> UniqueHashVals;
+  for (auto &SF : SFS) {
+    UniqueHashVals.clear();
+    for (auto &[IndexPair, Hash] : *SF->IndexOperandHashMap)


I am not aware that the unique values could differ, but it is still nice to count them precisely. This fix alone brings ~1% code size reduction on one of the projects I have tested.

nocchijiang · 2024-11-18T00:49:23Z

llvm/lib/CGData/StableFunctionMap.cpp

+    unsigned ParamCount = UniqueHashVals.size();
+    if (ParamCount > GlobalMergingMaxParams)
+      return false;
+    if (GlobalMergingSkipNoParams && ParamCount == 0)


So this pass acts as an IR-level ICF when ParamCount == 0? If your testing result shows it is beneficial then I have no problems with it.

I was also thinking this. To be explicit, does ICF get the same size win as when GlobalMergingMaxParams = 0? If so I think it would be worth saying this in the flag description to help users understand the pass a bit better.

To be explicit, does ICF get the same size win as when GlobalMergingMaxParams = 0?

No, this might actually increase the size since it always creates redundant thunks. However, depending on downstream passes or an improved linker's ICF, the outcome could be different. I have now set the flag to true to skip this ICF case, but an option remains available to enable it if needed.

nocchijiang · 2024-11-18T00:57:21Z

llvm/lib/CGData/StableFunctionMap.cpp

@@ -14,6 +14,7 @@
 //===----------------------------------------------------------------------===//

 #include "llvm/CGData/StableFunctionMap.h"
+#include "llvm/ADT/SmallSet.h"


nit: includes not sorted.

This is actually sorted correctly. The main header file (StableFunctionMap.h) should come first, corresponding to this file, StableFunctionMap.cpp. https://llvm.org/docs/CodingStandards.html#header-files

nocchijiang · 2024-11-18T01:03:22Z

llvm/lib/CGData/StableFunctionMap.cpp

+    "global-merging-inst-overhead",
+    cl::desc("The overhead cost associated with each instruction when lowering "
+             "to machine instruction."),
+    cl::init(1.0), cl::Hidden);


I am not sure if we should set the default value slightly higher, maybe like 1.2? I believe 1.0 is a guaranteed underestimation for AArch64 backend.

Just did a test with 1.2 and it delivered another 0.6% code size reduction on the same project I tested the effect of accurate ParamCount.

I don't mind changing the default value as it could eventually change depending on target/app. In theory, it's hard to precisely estimate final size impact for the given IR, anyhow,

ellishg · 2024-11-18T17:25:02Z

llvm/lib/CGData/StableFunctionMap.cpp

+    unsigned ParamCount = UniqueHashVals.size();
+    if (ParamCount > GlobalMergingMaxParams)
+      return false;
+    if (GlobalMergingSkipNoParams && ParamCount == 0)


I was also thinking this. To be explicit, does ICF get the same size win as when GlobalMergingMaxParams = 0? If so I think it would be worth saying this in the flag description to help users understand the pass a bit better.

llvm/test/CodeGen/Generic/cgdata-merge-no-params.ll

llvm/lib/CGData/StableFunctionMap.cpp

ellishg · 2024-11-19T19:20:07Z

llvm/lib/CGData/StableFunctionMap.cpp

+    if (ParamCount > GlobalMergingMaxParams)
+      return false;


As I understand if, if a group of mergable functions have too many params, we can't merge any of them. Could we eliminate functions until the params are small enough and only merge a subset of the group?

Correct. To compute the actual parameters, we need to iterate over the set of candidates with the same hash. By reducing these candidates, we might obtain a smaller set of parameters. However, this adds complexity to repeat these steps until we find a smaller set. In addition, since the overall function hash still matches, we may create unprofitable merging instances that the linker does not fold. I would maintain the current approach for simplicity.

kyulee-com · 2024-11-22T06:12:01Z

I believe all the comments have been addressed. Are there any more?"

kyulee-com · 2024-11-25T17:19:37Z

@nocchijiang @ellishg Can you take a look again? Thanks!

llvm-ci · 2024-11-25T19:04:11Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-sie-ubuntu-fast running on sie-linux-worker while building llvm at step 6 "test-build-unified-tree-check-all".

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