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"
+
+@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"
+
+@.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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
