[PATCH] D146777: [clang] Preliminary fat-lto-object support

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.



In D146777#4499608 <https://reviews.llvm.org/D146777#4499608>, @MaskRay wrote:

> Thanks for the update. Can you add a comment for the `-funified-lto` combination? It's unclear what it does...

ugh, I had put comments above each compiler invocation, but forgot that `#` makes lines ignored in the commit message. Thanks for pointing that out, as I hadn't noticed after updating.

> `clang -flto=thin -ffat-lto-objects -funified-lto -fuse-ld=lld foo.c`
>
> I've left some comments about missing test coverage.

ah, sorry, for some reason I read your last comment as `test locally to confirm it works` instead of the obvious `write a proper test`. I will correct that shortly.



================
Comment at: clang/test/CodeGen/embed-fat-lto-objects.c:3
+// RUN: %clang -cc1 -triple x86_64-unknown-linux-gnu -flto=full -ffat-lto-objects -emit-llvm < %s  | FileCheck %s --check-prefixes=FULL,SPLIT
+
+// RUN: %clang -cc1 -triple x86_64-unknown-linux-gnu -flto=thin -fsplit-lto-unit -ffat-lto-objects -emit-llvm < %s  | FileCheck %s --check-prefixes=THIN,SPLIT
----------------
MaskRay wrote:
> We need a `-emit-obj` test with `// REQUIRES: x86-registered-target`.
> Use llvm-readelf to check that .llvm.lto section is present.
> 
> We also need a `-S` test, otherwise the behavior of driver `clang -S -ffat-lto-objects` is untested.
> 
> 
Sure, I can do both of those, but as a general question, isn't the generation of assembly the responsibility of the backend? I believe we have some tests that use `llc` on the the modules that have gone through the FatLTO pipeline. Is that insufficient?

Also, as a best practice, do you know if there are specific types of changes to `clang` that imply that we'd need to test `-S`? Sorry for all the questions, but I'm trying to get a bit better in how I structure my patches and approach testing.


================
Comment at: clang/test/Driver/fat-lto-objects.c:10
+// CHECK-CC-NOLTO-NOT: -ffat-lto-objects
+// CHECK-CC-NOLTO-NOT: warning: argument unused during compilation: '-ffat-lto-objects'
+
----------------
MaskRay wrote:
> This NOT pattern has no effect as warnings are usually emitted before -cc1.
> 
> You can use `--implicit-check-not=warning:`
I'm happy to try that suggestion, but isn't this testing the generated `cc1` command from the driver(e.g.,  because we're using `%clang` and not `%clang_cc1`)? I //think// that should still produce the warning, shouldn't it? 

It's been a while since I made the patch, but I recall writing this test, having it fail, and then applying the change to stop it from being an `unused argument`... 

Also, thanks for all the great feedback on testing. Despite writing many(well, more than a few at least) `lit` tests, I'm still surprised by lots of behaviors, and your suggestions have been very helpful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146777