Page MenuHomePhabricator

[ThinLTO] Migrate ThinLTOBitcodeWriter to the new PM.
ClosedPublic

Authored by timshen on May 24 2017, 4:35 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen created this revision.May 24 2017, 4:35 PM
mehdi_amini added inline comments.May 24 2017, 5:07 PM
llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
423 ↗(On Diff #100183)

Can't you do it with a lambda?

timshen added inline comments.May 24 2017, 5:23 PM
llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
423 ↗(On Diff #100183)

I can, except that AAR needs to be allocated outside of the lambda, on the stack:

Optional<AAResults> AAR;
auto F = [&AAR](Function &) -> AAResults& { ... };

(AAR can't be captured by value, since AAResults is not copyable) this way AAR out-lives the lambda, and I feel less readable.

chandlerc edited edge metadata.May 25 2017, 2:28 AM

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
423 ↗(On Diff #100183)

You don't need to keep a copy though... You can directly expose the reference returned from the FunctionAnalysisManager -- that reference will stay alive until the result is invalidated. So this will become something more like:

[&FAM](Function &F) { return FAM.getResult<AAManager>(F); }
timshen added a comment.EditedMay 25 2017, 12:14 PM

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

I see that not all passes are registered (BitcodeWriterPass). Is it an oversight?

Since ThinkLTOBitcodeWriter takes a special raw_ostream to construct, I may end up with changing PassBuilder.cpp to make it registered.

pcc added a subscriber: pcc.May 25 2017, 1:00 PM
pcc added inline comments.
clang/test/CodeGen/thin_link_bitcode.c
5 ↗(On Diff #100183)

s/NEW_PM/NO_DEBUG/ and remove line 12.

I think you also want to test the contents of %t here.

timshen updated this revision to Diff 100462.May 26 2017, 1:21 PM

Change the test case.

timshen marked an inline comment as done.May 26 2017, 1:23 PM

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

Talked offline. Given the fact that BitcodeWriter (and possibly assembly writer?) is not registered either, it seems to be a larger issue that's out of the scope of this patch.

(focusing on the LLVM side of this review for now)

Can you add an LLVM-based test? Can you add this to lib/Passes/PassRegistry.def?

Talked offline. Given the fact that BitcodeWriter (and possibly assembly writer?) is not registered either, it seems to be a larger issue that's out of the scope of this patch.

While *generic* testing is out of scope, I think you need at least *some* testing of this in this patch. It can be an option directly in the llvm-lto tool or a direct option in the opt tool itself, but we shouldn't add a bunch of code to LLVM that can only ever be exercised by using some other tool.

timshen updated this revision to Diff 100488.May 26 2017, 4:00 PM

Add opt support and llvm test.

chandlerc added inline comments.May 26 2017, 7:23 PM
clang/lib/CodeGen/BackendUtil.cpp
913–914 ↗(On Diff #100488)

The clang side of this should probably be a separate review on cfe-commits, but one drive-by coment.

I find this usage of emplace much harder to read than:

ThinLinkOS.reset(raw_fd_ostream(CodeGenOpts.ThinLinkBitcodeFile, EC,
                                llvm::sys::fs::F_None));

Unless we just can't do the above for some reason (it won't compile =[) I would prefer it.

llvm/include/llvm/Transforms/IPO/ThinLTOBitcodeWriter.h
31–32 ↗(On Diff #100488)

Move (or add) a comment about this API to thea header? Notably, under what circumstances can ThinLinkOS be null? What happens if it is?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
9–12 ↗(On Diff #100488)

Can just omit the added section I suspect.

443 ↗(On Diff #100488)

Are we unable to deduce the return type here?

llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll
1 ↗(On Diff #100488)

Why run any passes here?

llvm/tools/opt/NewPMDriver.h
61 ↗(On Diff #100488)

Why an optional pointer? Maybe just allow the pointer to be null if there isn't a thin link output file?

llvm/tools/opt/opt.cpp
534 ↗(On Diff #100488)

Maybe unnecessary based on the above comment, but generally I would just assign rather than using emplace.

541–542 ↗(On Diff #100488)

We do Out->keep() in runPassPipeline, why not do this there as well?

timshen added inline comments.May 29 2017, 11:58 PM
llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll
1 ↗(On Diff #100488)

I saw that the runPassPipeline call is in if (PassPipeline.getNumOccurrences() > 0) {. I take that as "must specify -passes" in order to run the new pass manager (which is created in runPassPipeline). And specifiy an empty -passes cause an error message "unable to parse pass pipeline description".

llvm/tools/opt/NewPMDriver.h
61 ↗(On Diff #100488)

There are 3 states:

  1. Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the output file is not null.
  2. OutputThinLTOBC is true, but output file is null (don't write to the file).
  3. OutputThinLTOBC is false.

An optional nullable pointer, confusing as it might be (but I'm not sure how much), provides 3 states.

If we instead pass in a bool and a pointer, that's 4 states, one of which is invalid.

chandlerc added inline comments.May 30 2017, 1:02 AM
llvm/test/Transforms/ThinLTOBitcodeWriter/new-pm.ll
1 ↗(On Diff #100488)

You can use '-passes=no-op-module' or something similar to at least minimize how much occurs using the new PM?

llvm/tools/opt/NewPMDriver.h
61 ↗(On Diff #100488)

Ok, but is #2 in your list actually useful? Maybe it is, but if it is then at the very least the comment should make it super clear what is going on here. Otherwise, I suspect many readers will assume than when the optional is engaged the pointer isn't null as that's just too "obvious".

timshen updated this revision to Diff 100761.May 30 2017, 1:46 PM
timshen marked 12 inline comments as done.

Updated based on the comments, and left out Clang changes for a separate patch.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
443 ↗(On Diff #100488)

No, because return type deduction uses "auto deduction" instead of "decltype deduction" - it deduces to a value type.

llvm/tools/opt/NewPMDriver.h
61 ↗(On Diff #100488)

#2 is consistent with opt's legacy pass manager behavior (search "createWriteThinLTOBitcodePass" in opt.cpp).

I added an emphasized "*nullable*" in the comment. :)

timshen updated this revision to Diff 100911.May 31 2017, 1:19 PM

Rebase off from D33429. I realized that they are independent changes to LLVM.

Still not really happy with htis API, but let's fix it in a follow-up as it will grow the scope and much of the problem was already there (and the API is fairly narrow in scope).

llvm/tools/opt/NewPMDriver.h
61 ↗(On Diff #100488)

Ok, but this API is really, really confusing.

We should just add a new output kind to select between normal bitcode and thinlto bitcode. And then this can just be a pointer indicating if we want an additional thin link output.

But I'm happy to make that change in a follow-up (or for you to) as it'll need to change both PMs.

llvm/tools/opt/opt.cpp
532–534 ↗(On Diff #100911)

Inline this into the call?

OutputThinLTOBC ? ThinLinkOut.get : None

(maybe with {} around it...)

chandlerc accepted this revision.May 31 2017, 3:18 PM

Sorry, previous comment should have said LGTM with the tweak suggested.

This revision is now accepted and ready to land.May 31 2017, 3:18 PM
timshen updated this revision to Diff 100950.May 31 2017, 5:41 PM
timshen marked an inline comment as done.

Add ThinLTOBitcode as a new OutputKind to opt.

timshen added inline comments.May 31 2017, 5:44 PM
llvm/tools/opt/NewPMDriver.h
61 ↗(On Diff #100488)

Done in the same patch. It's actually not that many changes to add an OutputKind for the new PM in opt. The old PM in opt doesn't even use such an enum (that's probably why they have these booleans :).

llvm/tools/opt/opt.cpp
532–534 ↗(On Diff #100911)

It doesn't compile, because the two values provided by ?: should have the same type. :)

Also the Optional is gone. :)

timshen updated this revision to Diff 100952.May 31 2017, 5:53 PM

Keep the link file only when in ThinLTO bitcode mode.

chandlerc accepted this revision.May 31 2017, 6:00 PM

LGTM, much nicer API now thanks!

This revision was automatically updated to reflect the committed changes.