[PATCH] D146776: [llvm] Preliminary fat-lto-objects support

Mon Apr 17 17:23:15 PDT 2023

paulkirth added a comment.

In D146776#4276033 <https://reviews.llvm.org/D146776#4276033>, @aeubanks wrote:

> it'd be good to highlight somewhere that the thinlto-pre-link pipeline + module optimization pipeline is not exactly equivalent to the default pipeline (all the `Phase` params everywhere), so with this you'll get a different binary if you end up not going down the LTO route in the end than a typical non-fat build, especially if people ever want to adjust the thinlto pipeline (e.g. less aggressive inlining in the pre-link pipeline). the potential fiddling with the thinlto pre-link pipeline in the future is what has me most worried about this patch.

Noted. I'll look at adding some patch notes to go along with this that discuss the tradeoffs. There should be some kind of documentation in the `rst` files too, which is currently missing.

For what it's worth, I don't know if its a problem that they aren't identical. Ideally, they would be, but the only way to do that would be to compile twice, once for LTO and once for normal object code. @phosek and I discussed how support for that may look w/ @tejohnson and came to the conclusion that we probably don't want to add that type of complexity to clang. Plus, I don't think it's necessarily wrong to generate the IR as if we're doing LTO/ThinLTO and then complete the LTO/ThinLTO pipeline for the module.  Ideally, that would be extremely close to running the `perModuleDefaultPipeline`, but I think it's //probably// fine if they skew somewhat, since I don't imaging them straying too far. That said, I'm often wong, so I'm happy to defer to others on this point. :)

Is your worry here that we may move some kind of important transforms from prelink to post-link (or vise-versa)? Changing instrumentation or inlining are the ones that come to mind, but I'm sure there are probably lots of cases where altering the pipeline could be a problem. I'm just not sure they're any more important for this case more than other bitcode artifacts.

> (the commit description is out of date referencing `PerModuleDefaultPipeline`)

Good catch, thank you.

> I'd definitely like @tejohnson's feedback on this if there are any potential pitfalls we're missing

+100 to that

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776