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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the delay, couple more follow ups below.



================
Comment at: llvm/lib/LTO/LTO.cpp:1245
+      // These linkages are seen in Unified regular LTO, because the process
+      // of creating split LTO units introduces symbols with that linkage.
+      if ((LTOMode == LTOKind::LTOK_UnifiedRegular) &&
----------------
We can have split LTO units without UnifiedLTO, however. 


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


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


================
Comment at: llvm/test/LTO/Resolution/X86/unified-lto-check.ll:14
+; RUN: llvm-lto2 run -o %t3 %t1 %t2
+; RUN: not llvm-lto2 run --lto=thin -o %t3 %t1 %t2 2>&1 | \
+; RUN: FileCheck --allow-empty %s --check-prefix UNIFIEDERR
----------------
Since this option is only about UnifiedLTO and will give an error in this usage, it would be better to rename it to --unified-lto=.


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

The first 2 sentences contradict the second 2 I think? In any case, I think it makes sense to have a reasonable default, which seems to be implemented now.

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

See my comment in one of the tests, I think the option name should be clearer that it is just about UnifiedLTO.

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

I don't think the new debug messages being emitted and tested are correctly testing this, however, since runRegularLTO and runThinLTO are both essentially unconditionally invoked (see callsites in LTO::run). It would be better to add the messages to lto::backend and lto::thinBackend.


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

https://reviews.llvm.org/D123803