[PATCH] D123804: [WIP][clang][lld] A Unified LTO Bitcode Frontend

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1006
 
     if (IsThinLTO) {
       MPM = PB.buildThinLTOPreLinkDefaultPipeline(Level);
----------------
Simplify this with `if (IsThinLTO || (IsLTO && CodeGenOpts.UnifiedLTO))`


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4740
 
+  bool UnifiedLTO = Args.hasFlag(options::OPT_funified_lto,
+                                 options::OPT_fno_unified_lto, false);
----------------
Search for `Args.addOptInFlag`


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7227
   if (WholeProgramVTables) {
-    // Propagate -fwhole-program-vtables if this is an LTO compile.
-    if (IsUsingLTO)
-      CmdArgs.push_back("-fwhole-program-vtables");
+    bool IsPS4 = getToolChain().getTriple().isPS4();
+
----------------
Add a comment why PS4 is special.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4695
+            (D.getLTOMode() == LTOK_Full) || !UnifiedLTO)
+          CmdArgs.push_back("-flto-unit");
+      } else if (Triple.isAMDGPU()) {
----------------
tejohnson wrote:
> Why the PS4 special handling? Also, shouldn't LTO unit be enabled in unified LTO mode?
Do you want to use `isPS()` special case both PS4 and PS5? Or is it that just PS4 is special?

Is only PS4 is special, the summary should mention why.


================
Comment at: clang/test/CodeGen/asan-unified-lto.ll:8
+; RUN: %clang_cc1 -emit-llvm-bc -O1 -flto -funified-lto -fsanitize=address -o - -x ir < %s | llvm-dis -o - | FileCheck %s
+; CHECK: @anon.3ee0898e5200a57350fed5485ae5d237
+
----------------
Consider checking a few more things including the module flag metadata beside the global variable name.


================
Comment at: clang/test/CodeGen/asan-unified-lto.ll:15
+
+define i8* @f() {
+  %ptr = getelementptr inbounds [5 x i8], [5 x i8]* @.str, i32 0, i32 0
----------------
`ptr`. Avoid obsoleted typed pointers.


================
Comment at: clang/test/CodeGen/emit-summary-index.c:17
 
+// RUN: %clang_cc1 -flto=thin -funified-lto -emit-llvm-bc < %s | llvm-bcanalyzer -dump | FileCheck --check-prefix=UNITHIN %s
+// RUN: %clang_cc1 -flto -funified-lto -emit-llvm-bc < %s | llvm-bcanalyzer -dump | FileCheck --check-prefix=UNITHIN %s
----------------
Add a comment with `///` (for non-RUN non-CHECK lines) about the purpose of the two RUN lines.


================
Comment at: clang/test/CodeGen/unified-lto-pipeline.c:1
+// RUN: %clang -flto=thin -funified-lto -O2 -c %s -o %t.0
+// RUN: mv %t.0 %t.1
----------------
Without `--target`, this uses the default target triple which may fail for some exotic platforms.

Prefer `%clang_cc1` for non-driver tests. Instead of comparing the output files, test `-fdebug-pass-manager` output.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123804/new/

https://reviews.llvm.org/D123804