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

[Matthew Voss via Phabricator via llvm-commits]

ormris added inline comments.


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


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:950
           TheModule->addModuleFlag(Module::Error, "EnableSplitLTOUnit",
-                                   uint32_t(1));
+                                   !CodeGenOpts.UnifiedLTO ?
+                                   uint32_t(1) : !CodeGenOpts.UnifiedLTO);
----------------
tejohnson wrote:
> I think this ?: logic is all equivalent to simply setting the flag to !CodeGenOpts.UnifiedLTO. While as noted elsewhere I'd like to separate the split LTO unit handling from the unified LTO pipeline model, I'm not sure I understand why you would want to set this module flag to false when CodeGenOpts.UnifiedLTO==true under the current patch intent.
That's a good point. I've removed this change.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4740
 
+  bool UnifiedLTO = Args.hasFlag(options::OPT_funified_lto,
+                                 options::OPT_fno_unified_lto, false);
----------------
MaskRay wrote:
> Search for `Args.addOptInFlag`
There's driver code that depend on the value of this flag, so I do need to store it. `Args.addOptInFlag` doesn't return the value of the flag. Is there an advantage to calling addOptInFlag, then hasFlag?


================
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();
+
----------------
MaskRay wrote:
> Add a comment why PS4 is special.
Fixed.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4695
+            (D.getLTOMode() == LTOK_Full) || !UnifiedLTO)
+          CmdArgs.push_back("-flto-unit");
+      } else if (Triple.isAMDGPU()) {
----------------
MaskRay wrote:
> 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.
Fixed.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7048
+      (LTOMode == LTOK_Full || TC.canSplitThinLTOUnit()) ||
+       UnifiedLTO;
   bool SplitLTOUnit =
----------------
tejohnson wrote:
> I'd like to separate the split LTO unit setting from Unified LTO as I think they are orthogonal.
Fixed


================
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
----------------
MaskRay wrote:
> `ptr`. Avoid obsoleted typed pointers.
Fixed


================
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
----------------
MaskRay wrote:
> Add a comment with `///` (for non-RUN non-CHECK lines) about the purpose of the two RUN lines.
Fixed


================
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
----------------
MaskRay wrote:
> 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.
Why test the pipeline rather than binary equivalence? Identical bitcode files is a key part of this feature.


================
Comment at: clang/test/Driver/unified-whole-program-vtables.c:1
+// RUN: %clang -target x86_64-pc-linux-gnu -fwhole-program-vtables -funified-lto -### %s 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-pc-linux-gnu -fwhole-program-vtables -fno-unified-lto -### %s 2>&1 | FileCheck %s
----------------
MaskRay wrote:
> I think it'll be more readable if you place all -funitied-lto related driver tests in one file, instead of spreading them into unified-whole-program-vtables.c and lto-unit.c
> 
> Use `--target=` for new tests. `-target ` has been deprecated since Clang 3.4
Agreed. Fixed.


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

https://reviews.llvm.org/D123804