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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

There are a few comments that don't appear to be addressed, noted in the patch below, but also the below comment and question about split LTO - can you clarify whether that is still required for unified LTO? I also realized after going through the changes that there are a few things that need more testing, see comments.

In D123803#4411375 <https://reviews.llvm.org/D123803#4411375>, @tejohnson wrote:

> Looks pretty good but I have some mostly minor comments and questions.
>
> Patch summary needs slight change to remove this note since that got refactored out of this patch:
>
>> Make sure that the ModuleID is generated by incorporating more types of symbols.
>
> Also, is there still a requirement that split LTO units be enabled for Unified LTO? I cannot remember why that was the case, and I see some tests specify split LTO units and some don't. IMO it is better for split LTO and Unified LTO to be orthogonal if possible.





================
Comment at: llvm/lib/LTO/LTO.cpp:1148
+            || GV->hasAppendingLinkage()))
+        continue;
+
----------------
ormris wrote:
> tejohnson wrote:
> > tejohnson wrote:
> > > Why is this needed?
> > Ping on this question - please add comment about why this is needed.
> Fixed.
Specifically, why only in UnifiedRegular LTO mode?


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:278
+    WriteBitcodeToFile(M, OS, /*ShouldPreserveUseListOrder=*/false, &Index,
+                       false);
 
----------------
Document const parameter


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


================
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.
----------------
Is this a correct description? It seems to give an error, not silently handle it.


================
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 \
----------------
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).


================
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:
> 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.


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:182
+static cl::opt<std::string> UnifiedLTOMode("lto", cl::Optional,
+                                           cl::desc("Set LTO mode"),
+                                           cl::value_desc("mode"));
----------------
ormris wrote:
> tejohnson wrote:
> > Note the 2 accepted values in the message. Should it also accept "default"? Looks like the code will not, but we might want to for completeness.
> Yes, that makes sense. Fixed.
Needs a test.


================
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
Ping on why the CallGraphProfile is related to the Unified LTO setting. Especially now that the similar handling was removed from LTO.cpp.


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

https://reviews.llvm.org/D123803