Support VFE in thinLTO #69735

manman-ren · 2023-10-20T15:54:28Z

We add a run of GlobalDCEPass with ImportSummary. When ImportSummary is true, we remove virtual functions in vtables with VCallVisibility not Public. In this run, the regular GlobalDCEPass::AddVirtualFunctionDependencies will be bypassed, we will use ImportSummary to decide which virtual functions to remove.

Discussion points:
1> FuncsWithNonVtableRef: this is currently in ModuleSummaryIndex, does it
make sense to be part of FunctionSummary?
std::setGlobalValue::GUID FuncsWithNonVtableRef;
2> ComputeDependencies is copied from GlobalDCE to ModuleSummaryAnalysis, for the former,
ConstantDependenciesCache is a member variable
3> match from summary to Function in IR
Resolution is saved in std::setGlobalValue::GUID VFuncsToBeRemoved
Use F->getGUID() or GlobalValue::getGUID(F->getName()) when searching in VFuncsToBeRemoved

TODO:
1> support hybrid Regular/ThinLTO
2> support legacy thinLTO backend (ThinLTOCodeGenerator)

We add a run of GlobalDCEPass with ImportSummary. When ImportSummary is true, we remove virtual functions in vtables with VCallVisibility not Public. In this run, the regular GlobalDCEPass::AddVirtualFunctionDependencies will be bypassed, we will use ImportSummary to decide which virtual functions to remove. Discussion points: 1> FuncsWithNonVtableRef: this is currently in ModuleSummaryIndex, does it make sense to be part of FunctionSummary? std::set<GlobalValue::GUID> FuncsWithNonVtableRef; 2> ComputeDependencies is copied from GlobalDCE to ModuleSummaryAnalysis, for the former, ConstantDependenciesCache is a member variable 3> match from summary to Function in IR Resolution is saved in std::set<GlobalValue::GUID> VFuncsToBeRemoved Use F->getGUID() or GlobalValue::getGUID(F->getName()) when searching in VFuncsToBeRemoved TODO: 1> support hybrid Regular/ThinLTO 2> support legacy thinLTO backend (ThinLTOCodeGenerator)

teresajohnson

Thanks for the patch! I didn't go through in too much detail yet, but have some mostly higher level comments and suggestions.

teresajohnson · 2023-11-07T01:55:09Z

llvm/include/llvm/IR/ModuleSummaryIndex.h

@@ -1362,6 +1362,8 @@ class ModuleSummaryIndex {
  // Temporary map while building StackIds list. Clear when index is completely
  // built via releaseTemporaryMemory.
  std::map<uint64_t, unsigned> StackIdToIndex;
+  std::set<GlobalValue::GUID> FuncsWithNonVtableRef;


Instead of sets of GUIDs, it would probably be more efficient to represent, and to access, to just add new function flags (FFlags in the FunctionSummary).

teresajohnson · 2023-11-07T01:58:27Z

llvm/include/llvm/IR/ModuleSummaryIndex.h

@@ -1362,6 +1362,8 @@ class ModuleSummaryIndex {
  // Temporary map while building StackIds list. Clear when index is completely
  // built via releaseTemporaryMemory.
  std::map<uint64_t, unsigned> StackIdToIndex;
+  std::set<GlobalValue::GUID> FuncsWithNonVtableRef;
+  std::set<GlobalValue::GUID> VFuncsToBeRemoved; // no need to serialize


It would need to be serialized for distributed thinlto. A better thing would be to describe which of these are produced by the per-module analysis and used by the thin link, vs produced by the thin link and consumed by the ThinLTO backends. However, I suggest making these flags as noted above (but those should be documented with this information too)

teresajohnson · 2023-11-07T01:59:21Z

llvm/lib/Transforms/IPO/GlobalDCE.cpp

+    std::map<ValueInfo, std::vector<VirtFuncOffset>> &VFESafeVTablesAndFns,
+    std::map<ValueInfo, std::vector<ValueInfo>> &VFuncsAndCallers) {
+  // Go through Function summarys for intrinsics, also funcHasNonVTableRef to
+  // erase entries from VFESafeVTableAndFns.


why and when are entries being erased? More qualitative comments about what this algorithm is doing would be helpful.

teresajohnson · 2023-11-07T01:59:41Z

llvm/lib/Transforms/IPO/GlobalDCE.cpp

+        continue;
+      }
+
+      // Go through VS->vTableFuncs


Can you expand on the comment - what is this code doing and why?

teresajohnson · 2023-11-07T02:00:38Z

llvm/lib/Transforms/IPO/GlobalDCE.cpp

+  }
+  llvm::errs() << "VFEThinLTO number of TIds: " << NameByGUID.size() << "\n";
+
+  // VFESafeVTablesAndFns: map from VI for vTable to VI for vfunc


This comment seems to be in the wrong place, can you add a description of the below data structure (and describe VFESafeVTablesAndFns somewhere too, maybe where it is defined?)

teresajohnson · 2023-11-07T02:01:37Z

llvm/lib/Transforms/IPO/GlobalDCE.cpp

+      }
+    }
+  }
+  llvm::errs() << "VFEThinLTO number of vTables: " << vFuncSet.size() << " "


Should these be optimization remarks or debug messages?

teresajohnson · 2023-11-07T02:02:46Z

llvm/lib/Transforms/IPO/GlobalDCE.cpp

+  }
+}
+
+void runVFEOnIndex(ModuleSummaryIndex &Summary,


Can you add an overview of the algorithm as a comment above this?

teresajohnson · 2023-11-07T02:05:24Z

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

@@ -775,6 +783,58 @@ static void computeVariableSummary(ModuleSummaryIndex &Index,
  Index.addGlobalValueSummary(V, std::move(GVarSummary));
 }

+static void ComputeDependencies(


nit: function should be lowerCamelCase

teresajohnson · 2023-11-07T02:18:58Z

llvm/lib/Transforms/IPO/GlobalDCE.cpp

+    // again.
+    if (vfuncsRemoved.count(F->getGUID()))
+      return true;
+    if (vfuncsRemoved.count(GlobalValue::getGUID(F->getName())))


These lookups are a little dangerous as I think there are cases where we might rename and it wouldn't find the right function. For internalization we tag the function with an attribute before any thinlto renaming, linkage changes, or other optimization passes. See

llvm-project/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp

Line 271 in 0936a71

V->addAttribute("thinlto-internalize");

.

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' LTO Link time optimization (regular/full LTO or ThinLTO) llvm:ir llvm:analysis llvm:transforms labels Oct 20, 2023

manman-ren force-pushed the vfe-on-thinlto branch from 0468281 to 1adce63 Compare October 20, 2023 17:49

manman-ren requested review from teresajohnson, MaskRay and kubamracek October 20, 2023 17:53

teresajohnson reviewed Nov 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support VFE in thinLTO #69735

Support VFE in thinLTO #69735

manman-ren commented Oct 20, 2023

teresajohnson left a comment

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

teresajohnson Nov 7, 2023

Support VFE in thinLTO #69735

Are you sure you want to change the base?

Support VFE in thinLTO #69735

Conversation

manman-ren commented Oct 20, 2023

teresajohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment