[PATCH] D131618: [WIP][Do NOT review] LLD related changes for -ffat-lto-objects support

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7125-7127
+  // Currently FatLTO objects only embed ThinLTO bitcode
+  if (Args.hasArg(options::OPT_ffat_lto_objects))
+    CmdArgs.push_back("-flto=thin");
----------------
Rather than assuming ThinLTO, I think this patch should assume Unified LTO as was suggested in https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977.


================
Comment at: clang/test/Driver/fat-lto-objects.c:2-3
+// RUN: %clang -ccc-print-bindings -c %s -ffat-lto-objects 2>&1 | FileCheck %s
+// CHECK: clang
+// CHECK: clang
+
----------------
I don't think this test is useful, IIUC it checks that Clang accepts the `-ffat-lto-objects` flag, but that's also checked by the test below.


================
Comment at: lld/test/ELF/fatlto/Inputs/a-thinLTO.ll:1-66
+; ModuleID = 'a-thinLTO.bc'
+source_filename = "a.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at i = internal global i32 0, align 4
+
----------------
Rather than using the output of Clang which tends to include a lot of unnecessary stuff, I'd consider either writing the minimal `.ll` file manually, or pruning this one to drop everything that's not strictly needed for the test.


================
Comment at: lld/test/ELF/fatlto/Inputs/a.c:1-23
+#include "a.h"
+
+static signed int i = 0;
+
+void foo2(void) {
+  i = -1;
+}
----------------
This seems unnecessarily complicated. You should only include code that's absolutely necessary for the test.


================
Comment at: lld/test/ELF/fatlto/Inputs/a.h:1-3
+extern int foo1(void);
+extern void foo2(void);
+extern void foo4(void);
----------------
You can use forward declarations and avoid this header altogether.


================
Comment at: lld/test/ELF/fatlto/Inputs/main-thinLTO.ll:1-48
+; ModuleID = 'main-thinLTO.bc'
+source_filename = "main.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at .str = private unnamed_addr constant [4 x i8] c"Hi\0A\00", align 1
+
----------------
Rather than using the output of Clang which tends to include a lot of unnecessary stuff, I'd consider either writing the minimal `.ll` file manually, or pruning this one to drop everything that's not strictly needed for the test.


================
Comment at: lld/test/ELF/fatlto/Inputs/main.c:1-10
+#include <stdio.h>
+#include "a.h"
+
+void foo4(void) {
+  printf("Hi\n");
+}
+
----------------
This seems unnecessarily complicated. You should only include code that's absolutely necessary for the test.


================
Comment at: lld/test/ELF/fatlto/fatlto.test:4-6
+; clang -c -ffat-lto-objects -o a-fatLTO.o a.c
+; clang -c -ffat-lto-objects -o main-fatLTO.o main.c
+; clang -c a.c main.c
----------------
LLD tests cannot depend on `clang`, that's a layering violation. It also doesn't seem like the output is being used anywhere so this could be safely removed.


================
Comment at: lld/test/ELF/fatlto/fatlto.test:12
+; RUN: yaml2obj %p/Inputs/main-fatLTO.yaml > %tmain-fatLTO.o
+; RUN: llvm-readobj -S %ta-fatLTO.o | FileCheck --check-prefix=a %s
+; a: Name: .llvm.lto
----------------
The convention is to use uppercase names for prefixes.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:5080
+
+void llvm::embedBitcodeInFatObject(llvm::Module &M, llvm::MemoryBufferRef Buf) {
+  // Save llvm.compiler.used and remove it.
----------------
paulkirth wrote:
> Since this has been merged into this commit, I'll restate my concerns here from D131090.
> 
> Does this just do what embed bitcode does? My understanding was that LTO generated IR is emitted into object files differently than the embed bitcode scenario. I'm basing that on the discussion at https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977
> 
> What I mean is that the way `EmbedBitcode` worked was not `TheRightThing` according to that discussion. It appears as though your change just copies its logic.
> 
> So if I want to compile a C file and get a fat-LTO object, how does that work? When do the LTO passes get added to the compilation pipeline? I don't mean the big whole program optimization stuff that gets invoked at link-time, I mean the module level passes and summary generation? It's not clear to me when those will run after invoking clang on a source file, since I don't see any config for setting that up as part of codegen.
> 
> can you elaborate on how this is intended to work? I seem to be missing something. In particular, I'd like to some kind of documentation describing how all the pieces are intended to work. I'd also like to see something outlining any limitations, such as different combinations of object files/flags that are not expected to work and //why//?
I suspect that the approach we'll want to use is going to be akin to D130777 which uses a pass to control where in the pipeline the bitcode gets emitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131618