[PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

Tue Jan 14 13:38:19 PST 2020

tejohnson added a comment.

In D72624#1820281 <https://reviews.llvm.org/D72624#1820281>, @lenary wrote:

> In D72624#1817464 <https://reviews.llvm.org/D72624#1817464>, @tejohnson wrote:
>
> >
>
>
> Thank you for your feedback! It has been very helpful.
>
> > I'm not sure if ThinLTOCodeGenerator.cpp and LTOBackend.cpp were intentionally left out due to the LTO concerns mentioned in the description?
>
> Mostly left out because I wasn't sure how to attack them. I've got an update to this patch which I'm testing right now and it looks much better. I will post that imminently.
>
> > Note if we are just passing in the Module and updating the TM based on that, it wouldn't hit the threading issue I mentioned in D72245 <https://reviews.llvm.org/D72245>, but neither would you get the proper aggregation/checking across ThinLTO'ed modules.
>
> Ok, right, so I think I know what else this patch needs to do. At the moment, I think the `ModFlagBehavior` for module flags are not being checked during ThinLTO. I think this is something that has to be checked for compatibility in `ThinLTOCodeGenerator::addModule` (like the triple is checked for compatibility).

And LTO::addModule (just to add confusion, there are two LTO APIs, ThinLTOCodeGenerator is the old one and LTO is the new one, the latter being used by lld and the gold plugin).

I had mentioned using LTO::addModule to do the checking in the other patch, but there is a complication I should mention:

> I see that the checking behaviour is in `IRMover`, but I don't think ThinLTO uses that, and I don't feel familiar enough with ThinLTO to be sure.

The ThinLTO "link", which is where the modules are added serially, does not read IR, only the summaries, which are linked together into a large index used to drive ThinLTO whole program analysis. So you can't really read the module flags directly during addModule, they need to be propagated via the summary flags. The ThinLTO backends which are subsequently fired off in parallel do read IR. In those backends, depending on the results of the ThinLTO analysis phase, we may use IRMover to link in ("import) functions from other modules. At that point, the module flags from any modules that backend is importing from will be combined and any errors due to conficting values will be issued.

Thinking through this some more, rather than attempting to fully validate the consistency of the module flags across all modules in ThinLTO mode, just rely on some checking when we merge subsections of the IR in the ThinLTO backends during this importing, which will happen automatically. This is presumably where the checking is desirable anyway (in terms of the cases you are most interested in catching with ThinLTO, because the IR is getting merged). Note that unlike in the full LTO case, where the IR is merged before you create the TM, in the ThinLTO case the TM will be created before any of this cross-module importing (partial IR merging), so with your patch presumably it will just use whatever module flag is on that original Module for it's corresponding ThinLTO backend. But since it sounds like any difference in these module flags is an error, it will just get flagged a little later but not affect how the TM is set up in the correct case. Does that sound reasonable?

> The update to my patch will not address this part of ThinLTO.

I'll take a look through your patch later today or tomorrow, but it may be just fine from the above perspective.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624