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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2929
           NotDiscardableComdats.insert(C);
-    for (Function &F : M)
+    for (Function &F : M) {
+      if (F.getName() == "__stack_chk_fail" ||
----------------
I'm not sure if this is the right place to do this, since it doesn't need to be done iteratively, only once. I.e. this code is executed "while (LocalChange)".

Wondering if other reviewers have thoughts on where the right place to add these to the used is.


================
Comment at: test/ThinLTO/X86/builtin-nostrip.ll:1
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/builtin-nostrip.ll -o %t2.bc
----------------
You will probably need to add a line like:
; REQUIRES: x86-registered-target

to prevent non-x86 bot failures.


================
Comment at: test/ThinLTO/X86/builtin-nostrip.ll:15
+; RUN:   -r %t1.bc,memcpy,pl \
+; RUN:   -r %t2.bc,boo,pl
+
----------------
Need to add an llvm-nm invocation on the correct object file emitted by llvm-lto2. Otherwise you aren't doing any checking like you are for llvm-lto above.


================
Comment at: test/ThinLTO/X86/builtin-nostrip.ll:22
+
+; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t.out -save-temps \
+; RUN:   -r %t1.bc,main,plx \
----------------
This llvm-lto2 invocation is exactly the same as the prior one - I don't think you should do this twice?


================
Comment at: test/ThinLTO/X86/builtin-nostrip.ll:51
+; RUN:   -r %t1.bc,memcpy,pl \
+; RUN:   -r %t3.bc,boo,pl 
+
----------------
Needs some kind of FileCheck on one of the output files, similar to my earlier comment on the first llvm-lto2 invocation.


https://reviews.llvm.org/D49434