[PATCH] D52966: [Merge SImilar Function ThinLTO 3/n] Add hash code to function summary

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hi @hiraditya , pardon my ignorance in this part of llvm. Most of my comments are questions about your reasoning.



================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:73
 
-static cl::opt<bool> UseGlobalAliases(
-    "mergesimilarfunc-global-aliases", cl::Hidden, cl::init(false),
-    cl::desc("Enable writing alias by enabling global aliases"));
+extern cl::opt<bool> UseGlobalAliases;
+extern cl::opt<unsigned> MergeMinInsts;
----------------
Why did you change these to externs?


================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:89
+namespace llvm {
+unsigned profileFunction(const Function *F);
+}
----------------
Where is this function defined now?


================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:92
 
+/*
 /// Returns the type id for a type to be hashed. We turn pointer types into
----------------
Was this intentional, to add this diff with the entire profile function commented out?


================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:1137
     unsigned Hash = profileFunction(F);
+    if (Hash == 0)
+      continue;
----------------
Maybe comment here that 0 is a placeholder/invalid hash?


================
Comment at: lib/Transforms/IPO/MergeSimilarFunctions.cpp:1169
+  unsigned Hash = profileFunction(F);
+  if (Hash == 0)
     return;
----------------
(same comment)


https://reviews.llvm.org/D52966