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

[Matthew Voss via Phabricator via llvm-commits]

ormris added inline comments.


================
Comment at: llvm/lib/LTO/LTO.cpp:739
+  if (LTOMode == LTOK_UnifiedRegular)
+    if (NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions"))
+      M.eraseNamedMetadata(CfiFunctionsMD);
----------------
tejohnson wrote:
> Why is this needed?
If cfi.functions isn't removed, LowerTypeTests will rename local functions in the merged module as "<function name>.1" when the regular LTO backend is used. This causes linking errors, since other parts of the module expect the original function name. We saw this happen in internal testing.


================
Comment at: llvm/lib/LTO/LTO.cpp:1128
+  if (LTOMode == LTOKind::LTOK_UnifiedRegular)
+    EnableLTOInternalization = true;
+
----------------
tejohnson wrote:
> Why is this needed?
Looks like it's not needed. I'll remove it.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:212
     PerformThinLTO = EnablePerformThinLTO;
+    UnifiedLTO = false;
     DivergentTarget = false;
----------------
tejohnson wrote:
> Rather than adding the many checks in the file below, can the Perform* and PrepareFor* options just be initialized differently under the UnifiedLTO mode?
No, I don't think so. The UnifiedLTO flag doesn't match any of those variables. I don't see a combination Perform*/PrepareFor* that would cleanly produce the result we want. I would also worry that reusing these variables would make this code less clear. Looking at it now, I wonder if it should be called `PrepareForUnifiedLTO`, though.


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

https://reviews.llvm.org/D123803