[PATCH] D49434: Put "built-in" function definitions in global Used list, for LTO. (fix bug 34169)

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/Object/IRSymtab.cpp:235
 
-  if (Used.count(GV))
+  bool isBuiltinFunc = false;
+
----------------
IsBuiltinFunc


================
Comment at: lib/Object/IRSymtab.cpp:237
+
+  // FIXME:  This fixes the issue with builtin functions being
+  // internalized (and later dead-code-eliminated) in the new LTO
----------------
I don't think we need this FIXME. As far as I know, this problem is already "solved" in the old LTO API by the client providing an appropriate preserve list.


================
Comment at: lib/Object/IRSymtab.cpp:241
+  // which still internalizes the function definitions.
+  for (const char *libcall_name : LibcallRoutineNames)
+    if (GV->getName() == libcall_name)
----------------
LibcallName



================
Comment at: test/ThinLTO/X86/builtin-nostrip.ll:1
+; REQUIRES: x86-registered-target
+; Compile with thinlto indices, to enable thinlto.
----------------
A lot of this test case can be removed I think.
- You just need one libcall function, testing more than one wouldn't give you additional coverage.
- A lot of the test input (including the second input file) is irrelevant to what you're trying to test. Basically I think this test just needs two functions, both with resolution `p`, one which is a libcall and one which isn't, and you would test that the first one is kept and the second one isn't.
- Remove the parts about the old LTO API because the problem is solved in a different way there.


https://reviews.llvm.org/D49434