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.

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to