[PATCH] D123803: [WIP][llvm] A Unified LTO Bitcode Frontend
Matthew Voss via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 12 16:24:40 PDT 2023
ormris added inline comments.
================
Comment at: llvm/include/llvm/LTO/Config.h:176
+ bool UnifiedLTO = false;
+
----------------
tejohnson wrote:
> 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?
I can derive this from other values. Removed.
================
Comment at: llvm/include/llvm/LTO/LTO.h:258
public:
+ enum LTOKind {
+ LTOK_Default,
----------------
tejohnson wrote:
> Document
Fixed.
================
Comment at: llvm/include/llvm/LTO/LTO.h:431
+ LTOKind LTOMode;
+
----------------
tejohnson wrote:
> Document
Fixed.
================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:77
+ // Add LTO pipeline tuning option to enable our unified LTO pipeline.
+ bool UnifiedLTO;
----------------
tejohnson wrote:
> s/our/the/ ?
Fixed.
================
Comment at: llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h:34
+ ThinLTOBitcodeWriterPass(raw_ostream &OS, raw_ostream *ThinLinkOS,
+ bool UnifiedLTO = false)
: OS(OS), ThinLinkOS(ThinLinkOS) {}
----------------
tejohnson wrote:
> This isn't used - remove?
Fixed
================
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.
----------------
tejohnson wrote:
> 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.
Yeah, I see what you mean. I think it would be best to make this function return a tuple. Fixed.
================
Comment at: llvm/lib/LTO/LTO.cpp:739
+ if (LTOMode == LTOK_UnifiedRegular)
+ if (NamedMDNode *CfiFunctionsMD = M.getNamedMetadata("cfi.functions"))
+ M.eraseNamedMetadata(CfiFunctionsMD);
----------------
tejohnson wrote:
> 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).
Fixed.
================
Comment at: llvm/lib/LTO/LTO.cpp:1128
+ if (LTOMode == LTOKind::LTOK_UnifiedRegular)
+ EnableLTOInternalization = true;
+
----------------
tejohnson wrote:
> 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.
Fixed.
================
Comment at: llvm/lib/LTO/LTO.cpp:1148
+ || GV->hasAppendingLinkage()))
+ continue;
+
----------------
tejohnson wrote:
> tejohnson wrote:
> > Why is this needed?
> Ping on this question - please add comment about why this is needed.
Fixed.
================
Comment at: llvm/lib/LTO/LTO.cpp:1547
+ if (Conf.UnifiedLTO)
+ Conf.PTO.CallGraphProfile = false;
+
----------------
tejohnson wrote:
> tejohnson wrote:
> > Why is this needed?
> Ping on this question. Please add comment about why needed.
Now that we're using the ThinLTO pre-link pipeline, we can remove this.
================
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));
----------------
tejohnson wrote:
> Add comment summarizing the decision/rationale for this.
Fixed
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:278
+ WriteBitcodeToFile(M, OS, /*ShouldPreserveUseListOrder=*/false, &Index,
+ /*GenerateHash=*/UnifiedLTO);
----------------
tejohnson wrote:
> Since you are asserting that UnifiedLTO is false a few lines up, can this just be a constant false?
Fixed
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:284
WriteBitcodeToFile(M, *ThinLinkOS, /*ShouldPreserveUseListOrder=*/false,
- &Index);
+ &Index, UnifiedLTO);
----------------
tejohnson wrote:
> Diito
Fixed
================
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
+
----------------
tejohnson wrote:
> 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.
Fixed
================
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
----------------
tejohnson wrote:
> Does the test need to include the textual summary, or will the correct summary be generated with -thinlto-bc?
No, that's not needed. Fixed.
================
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.
----------------
tejohnson wrote:
> 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")
Yes, it was. This is a very old test for a crash that's long been fixed. Essentially, the issue was that type test instructions were not being removed, and that caused crashes during codegen. Honestly, I think it would be best to keep this test private. It's good for our internal test suite, but I'm not sure it adds value here.
================
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
----------------
tejohnson wrote:
> Similar question to the other test - do we need to include the textual summary or does it get automatically generated by -thinlto-bc?
It's auto-generated. I'll remove this.
================
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:
> Add comment at the top about what the test is testing (it isn't clear to me).
Fixed.
================
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"));
----------------
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.
================
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));
----------------
tejohnson wrote:
> Note that it is ignored unless -thinlto-bc specified
Fixed
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123803/new/
https://reviews.llvm.org/D123803
More information about the llvm-commits
mailing list