[PATCH] D123803: [WIP][llvm] A Unified LTO Bitcode Frontend
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 10 10:35:56 PDT 2023
tejohnson added a comment.
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/include/llvm/LTO/Config.h:176
+ bool UnifiedLTO = false;
+
----------------
Document. However, the only use I could find of this field is immediately after it is set, in the same scope. Does it need to be a Config field?
================
Comment at: llvm/include/llvm/LTO/LTO.h:258
public:
+ enum LTOKind {
+ LTOK_Default,
----------------
Document
================
Comment at: llvm/include/llvm/LTO/LTO.h:431
+ LTOKind LTOMode;
+
----------------
Document
================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:77
+ // Add LTO pipeline tuning option to enable our unified LTO pipeline.
+ bool UnifiedLTO;
----------------
s/our/the/ ?
================
Comment at: llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h:34
+ ThinLTOBitcodeWriterPass(raw_ostream &OS, raw_ostream *ThinLinkOS,
+ bool UnifiedLTO = false)
: OS(OS), ThinLinkOS(ThinLinkOS) {}
----------------
This isn't used - remove?
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:8052
+ case BitstreamEntry::EndBlock: {
// If no flags record found, conservatively return true to mimic
// behavior before this flag was added.
----------------
Comment needs update. Also, what should the value of UnifiedLTO be set to in this case? I suppose it defaults to false, which seems correct, but it would be good to explicitly set/note that.
I think though it might be better to change this function to return a tuple of the 2 flags, since there are other fields in the BitcodeLTOInfo that are not being set here. I see that they are set by the caller, but this is a bit confusing IMO. Alternatively, change this function to a name "setEnable..." (s/get/set), and note explicitly in a comment above the function that the caller is expected to set the other fields.
================
Comment at: llvm/lib/LTO/LTO.cpp:739
+ if (LTOMode == LTOK_UnifiedRegular)
+ if (NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions"))
+ M.eraseNamedMetadata(CfiFunctionsMD);
----------------
ormris wrote:
> 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.
Please document this rationale in a comment, and note that this metadata is only needed for ThinLTO (which appears to be the case).
================
Comment at: llvm/lib/LTO/LTO.cpp:1128
+ if (LTOMode == LTOKind::LTOK_UnifiedRegular)
+ EnableLTOInternalization = true;
+
----------------
ormris wrote:
> tejohnson wrote:
> > Why is this needed?
> Looks like it's not needed. I'll remove it.
Ping on this question, I think it should be removed? EnableLTOInternalization is an internal option that defaults to true anyway.
================
Comment at: llvm/lib/LTO/LTO.cpp:1148
+ || GV->hasAppendingLinkage()))
+ continue;
+
----------------
tejohnson wrote:
> Why is this needed?
Ping on this question - please add comment about why this is needed.
================
Comment at: llvm/lib/LTO/LTO.cpp:1547
+ if (Conf.UnifiedLTO)
+ Conf.PTO.CallGraphProfile = false;
+
----------------
tejohnson wrote:
> Why is this needed?
Ping on this question. Please add comment about why needed.
================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1319
} else if (Matches[1] == "lto-pre-link") {
- MPM.addPass(buildLTOPreLinkDefaultPipeline(L));
+ if (PTO.UnifiedLTO)
+ MPM.addPass(buildThinLTOPreLinkDefaultPipeline(L));
----------------
Add comment summarizing the decision/rationale for this.
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:278
+ WriteBitcodeToFile(M, OS, /*ShouldPreserveUseListOrder=*/false, &Index,
+ /*GenerateHash=*/UnifiedLTO);
----------------
Since you are asserting that UnifiedLTO is false a few lines up, can this just be a constant false?
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:284
WriteBitcodeToFile(M, *ThinLinkOS, /*ShouldPreserveUseListOrder=*/false,
- &Index);
+ &Index, UnifiedLTO);
----------------
Diito
================
Comment at: llvm/test/LTO/Resolution/X86/local-def-dllimport.ll:1
-; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t0.bc %s
+; RUN: opt --unified-lto -thinlto-split-lto-unit -thinlto-bc -o %t0.bc %s
+
----------------
Why change this test? I assume it should still work with the old options. If you want to test also with Unified LTO, just duplicate the RUN lines so that it tests in both modes.
================
Comment at: llvm/test/LTO/X86/unified-cfi.ll:88
+
+^0 = module: (path: "llvm/test/LTO/X86/unified-cfi.ll", hash: (0, 0, 0, 0, 0))
+^1 = gv: (name: "llvm.type.test") ; guid = 608142985856744218
----------------
Does the test need to include the textual summary, or will the correct summary be generated with -thinlto-bc?
================
Comment at: llvm/test/LTO/X86/whole-program-no-crash.ll:2
+; Run the ThinLTO and LTO backends on a module with
+; devirtualizaiton metadata. In previous versions of the compiler,
+; this crashed.
----------------
Is this comment about crashing in previous versions of the compiler copied from another test? Is the crash related to unified LTO somehow?
(also typo in "devirtualizaiton")
================
Comment at: llvm/test/LTO/X86/whole-program-no-crash.ll:90
+
+^0 = module: (path: "llvm/test/LTO/X86/whole-program-no-crash.ll", hash: (160140095, 1084170952, 2125434145, 3248440305, 919813895))
+^1 = gv: (name: "llvm.type.test") ; guid = 608142985856744218
----------------
Similar question to the other test - do we need to include the textual summary or does it get automatically generated by -thinlto-bc?
================
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 \
----------------
Add comment at the top about what the test is testing (it isn't clear to me).
================
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
----------------
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.
================
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
----------------
Why this checking?
================
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"));
----------------
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.
================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:367
+ Conf.UnifiedLTO = (LTOMode != LTO::LTOK_Default);
+ Conf.PTO.CallGraphProfile = !Conf.UnifiedLTO;
+
----------------
Needs comment about why
================
Comment at: llvm/tools/opt/opt.cpp:120
+static cl::opt<bool> UnifiedLTO("unified-lto",
+ cl::desc("Use unified LTO piplines"),
+ cl::Hidden, cl::init(false));
----------------
Note that it is ignored unless -thinlto-bc specified
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123803/new/
https://reviews.llvm.org/D123803
More information about the llvm-commits
mailing list