[PATCH] D123803: [WIP][llvm] A Unified LTO Bitcode Frontend

[Matthew Voss via Phabricator via llvm-commits]

ormris added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:278
+    WriteBitcodeToFile(M, OS, /*ShouldPreserveUseListOrder=*/false, &Index,
+                       false);
 
----------------
tejohnson wrote:
> Document const parameter
Fixed? Does this need more explanation?


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:284
       WriteBitcodeToFile(M, *ThinLinkOS, /*ShouldPreserveUseListOrder=*/false,
-                         &Index);
+                         &Index, false);
 
----------------
tejohnson wrote:
> Ditto
Fixed?


================
Comment at: llvm/test/LTO/Resolution/X86/unified-lto-check.ll:2
+; Test to ensure that the Unified LTO flag is set properly in the summary, and
+; that we correctly silently handle linking bitcode files with different values
+; of this flag.
----------------
tejohnson wrote:
> Is this a correct description? It seems to give an error, not silently handle it.
Fixed


================
Comment at: llvm/test/ThinLTO/X86/dup-cgprofile-flag.ll:1
+; RUN: opt <%s -unified-lto -thinlto-bc -thinlto-split-lto-unit -o %t0
+; RUN: llvm-lto2 run %t0 --lto=full -o %t1 \
----------------
tejohnson wrote:
> ormris wrote:
> > tejohnson wrote:
> > > Add comment at the top about what the test is testing (it isn't clear to me).
> > Fixed.
> I'm still unclear as to what is happening in the test. It seems that there is an error when running LTO without specifying a --lto= option. It isn't clear to me why, or why specifically that case duplicates a module flag.
> 
> This raises a couple questions:
> 1) is it unsupported to LTO link unifiedLTO IR objects without specifying a non-default LTO mode via --lto=[thin|full]?
> 2) is it unsupported to specify an LTO mode other than default via --lto=[thin|full] for non-unifiedLTO IR objects?
> 
> The unified-lto-check.ll test does test a few of these option combinations, but not all of them. There should be a test for all combinations, with a clear error for any that are not supported. IMO it might be nice to handle case 1 silently with a reasonable default (probably ThinLTO since that's the pre-link pipeline used).
> 
> It would also be good to have a test that more explicitly ensures that we get the expected ThinLTO vs RegularLTO backend handling for unifiedLTO IR objects with both --lto=thin and --lto=full (maybe this exists, but I don't see such a test right now scanning the patch again).
OK. I've added further details to the comment. Let me know if that makes sense.

> is it unsupported to LTO link unifiedLTO IR objects without specifying a non-default LTO mode via --lto=[thin|full]?
I think that should be unsupported. Otherwise, small pipeline differences could catch users by surprise. A default of ThinLTO does make sense. Fixed.

> is it unsupported to specify an LTO mode other than default via --lto=[thin|full] for non-unifiedLTO IR objects?
It probably should be. There's a chance that someone could use the switch by accident and get a strange result. Fixed.

> There should be a test for all combinations
Agreed. I've added the rest of these cases to unified-lto-check.ll.

> It would also be good to have a test that more explicitly ensures that we get the expected ThinLTO vs RegularLTO backend
Yes, that would be useful. Fixed.


================
Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/split-unified.ll:2
+; Generate bitcode files with summary, as well as minimized bitcode without
+; the debug metadata for the thin link.
+; RUN: opt -unified-lto -thinlto-bc -thin-link-bitcode-file=%t2 -o %t %s
----------------
tejohnson wrote:
> tejohnson wrote:
> > Can you add some checking of the generated minimized bitcode file %t2? Also, it is not just without the debug metadata, it is without all IR.
> ping on the comments in this test.
Sorry for the delay here. This test is named incorrectly. It was intended to test the case where the ModuleID is not generated. Since we've removed that case from discussion, I've changed this test to cover the normal case.



================
Comment at: llvm/test/Transforms/ThinLTOBitcodeWriter/split-unified.ll:6
+; RUN: llvm-modextract -b -n 0 -o %t0.bc %t
+; RUN: not llvm-modextract -b -n 1 -o - %t 2>&1 | FileCheck --check-prefix=ERROR %s
+; RUN: llvm-dis -o - %t0.bc | FileCheck --check-prefix=M0 %s
----------------
tejohnson wrote:
> Why this checking?
See above


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:367
+  Conf.UnifiedLTO = (LTOMode != LTO::LTOK_Default);
+  Conf.PTO.CallGraphProfile = !Conf.UnifiedLTO;
+
----------------
tejohnson wrote:
> Needs comment about why
This can also be removed, since we're using the ThinLTO pre-link pipeline.


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

https://reviews.llvm.org/D123803