LGTM.

In D34197#784285, @ilya-biryukov wrote:

Feel free to advocate for it, but except that stating an opinion above I'm missing you arguments.

For a newcomer or even a contributor, not already familiar with LLVM's build system, it's not at all obvious which arguments one must pass to cmake to make build work for him.

LGTM with an inline comment

In D34197#783750, @ilya-biryukov wrote:

I would advocate for an easier interface to the build script, even if it would add some maintenance costs (that said, I'm ready to maintain it).
I think having a script, accepting parameters like 'make a 2-stage bootstrap and install lld+clang)', is a better interface than 'checkout projects cfe, lld, pass arguments "-X -Y -Z" to cmake'. Keeping an interface to the build script simple (while also not letting the maintenance costs blow up) should be a priority here, IMO.

I'm interested into seeing this landing! However I also have concerns right now:

Thanks for fixing! (I'm still not the best qualified to review this)

(If @efriedma thinks that the fix is the right one for the provided explanation that's fine with me)

OK! Thanks both :)

If you need examples about what I'm expecting to read from a commit message (same applies to comment in the code though), you may look at these commit messages:

In D33515#780816, @marsupial wrote:

From the summary:

Fixes an issue using RegisterStandardPasses from a statically linked object before PassManagerBuilder::addGlobalExtension is called from a dynamic library.

What would you like either the comment or the summary it to say?

In D34156#780570, @tobiasvk wrote:

• Change patch to always emit a module summary for regular LTO
  
  I don't see any real downside of having a summary given the marginal time and space overhead it takes to construct and save it.

In D33515#780599, @marsupial wrote:

I'm missing the *why* using a "real function" (I guess you meant "function pointer") matter?

Don't understand this.
Is the comment really confusing/unhelpful?

Missing LangRef update.

In D33976#776486, @mehdi_amini wrote:

In D33976#775918, @alexshap wrote:

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

I'll CC you on the bug when I get time to reduce the test case and file the bug.

LGTM.

In D33976#775918, @alexshap wrote:

@mehdi_amini , thanks, i see, regarding the "opposite issue" - probably an example / test case would be helpful, that looks like a separate issue.
Thanks for adding @ahatanak and @arphaman, that would be wonderful if smb could look at this diff (which, besides the fix, adds tests)

I'm fine either way with this patch, and @chandlerc didn't answer my last comment.

Interestingly I got the opposite issue recently: calling through a macro with a single format specifier was *adding* new specific in the fixit in some conditions.

LGTM

LGTM, thanks.

(Clarification the "code" part LGTM, so as soon as Sanjoy is OK with LangRef, that's fine with me)

LGTM

Could be useful, but that seems a bit intrusive right now (ad-hoc hooks in Module and PassManager)

In D33513#772844, @ncharlie wrote:

Addressed comments. Formatted with clang-format, so that's why there are so many extra changes in the diff.

This commit was part of the series to get rid of ThinLTOCodeGenerator. Removing this would makes the current state of the "new LTO" further away of the features exposed by ThinLTOCodeGenerator.

Thanks for splitting out a large part of the refactoring!

In D33125#769930, @chandlerc wrote:

Honestly, the more I think about this, the more I think that the complexity it involves doesn't pull its weight.

In D33615#769259, @eugenis wrote:

This flag is not serialized. We don't need it to be serialized, but that same is true for Live / LiveRoot. Actually, LiveRoot serialization was added in D23488 along with bitcode version bump, and, as far as I can see, it was unused from the start. I can kill the whole thing, and maybe even downgrade the version, if no one minds.

To expand: my current concern is that the issue spotted with "instance" of scope makes the whole mechanism entirely opaque now, I'm not longer sure what is the point of specifying it at the IR level in a target-independent way.
@t-tye you were the one mentioning some generic understanding of the code as one of the original motivation IIRC?

In D21723#769140, @t-tye wrote:

LGTM

In D33615#768262, @eugenis wrote:

To recap: this needs to be in combined summary, because that's the only way to pass information to LowerTypeTests in regularLTO.

In D33615#766506, @pcc wrote:

In D33615#766505, @mehdi_amini wrote:

Add a bit to GVFlags to tell ThinLTO backends which values are dead.

Why?

See Evgeniy's RFC thread, in particular
http://lists.llvm.org/pipermail/llvm-dev/2017-May/113298.html

In D33540#766416, @chandlerc wrote:

Yeah, sorry, this all only makes sense for *partial inlining*, not for the rest of it. Anyways... Sorry for confusion.

Add a bit to GVFlags to tell ThinLTO backends which values are dead.

LGTM.

Indeed, looks like the buildModuleSimplificationPipeline got me confused with buildFunctionSimplificationPipeline, sorry /me -> []

Note: the plan was to tune the inliner heuristic in the compile phase differently from the thin-backends phase, but we never got to it. It is also already challenging to maintain and evolve the existing thresholds, that I'm not sure adding another "optimization goal" is sustainable.

In D33540#765031, @chandlerc wrote:

In D33540#764937, @davidxl wrote:

For partial inlining, leave it consistent with old PM for now. We can introduce later an internal option to enable/disable pre-thin-link partial inlining and collect more performance number.

Is there a rationale for the position in the old PM? Was this discussed somewhere?

In D33513#764629, @ncharlie wrote:

In D33513#763708, @davide wrote:

Thanks for your first contribution, great GSoC start!

Thanks! This was a good chance to look closely at the code to start learning how it works.

One note: this manifests with ThinLTO, but it might be a more generic metadata issue. Are you able to reproduce without ThinLTO (and maybe with a single file?)

The bug only happens when lazy loading Metadata. Lazy loading occurs when module level metadata is being imported by ThinLTO (see https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/lib/Bitcode/Reader/MetadataLoader.cpp;303885$781). I believe this is the only path that leads to lazy loading, so I don't think it can happen elsewhere.

LGTM.

It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier.

LGTM.

Thanks!

LGTM.

LGTM. See one inline comment.

In D33151#757534, @aprantl wrote:

In D33151#756851, @aprantl wrote:

In D33151#756801, @mehdi_amini wrote:

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

Thanks for the offer! Please let me know if you won't get to it. If you can just quickly sketch what needs to be done I can do the grunt work, too.

In D33151#756793, @aprantl wrote:

In D33151#756788, @mehdi_amini wrote:

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

In D33151#756783, @aprantl wrote:

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

In the attached version I removed the flag. This will cause the

void ThinLTOCodeGenerator::run() // CodeGenOnly=true

path to verify the module twice. I'm not particularly concerned by this though and it keeps the code simpler.

(thanks, with this naming it looks better to me!)

(+@pcc)
Someone should take care of making sure this is also handled by the new LTO API.

I think so, but what would be nice to have a test that round-trip a a few non-standard strings (i.e. not "singlethread" or "system") through MIR and through bitcode.

Is there a MIR test?

LGTM, thanks.

LGTM.

In D33162#754180, @pcc wrote:

Most of the uses reads quite wrong since it has nothing to do with PGO.

Yes, that's the point :)

Use of a function named getPGOName() in code that has nothing to do with PGO stands out and makes it more obvious that there is a bug. Using a "better" name would increase the chance of the function being used incorrectly in other places.

Most of the uses reads quite wrong since it has nothing to do with PGO. I'm not fond of this renaming right now, can we find another alternative name?

What about disabling the verification as part of the pass pipeline and just do the manual call to the verifier on loading?

The codegen-only path also does not exercise the cross-module importing.

LGTM

In D33030#751723, @ahatanak wrote:

If field unwindHeader is annotated with the aligned attribute, both the field and struct cxa_exception are aligned. As a result, the exception object that follows cxa_exception is aligned too.

If we annotate struct cxa_exception with the attribute instead of annotating the field, the test case segfaults. It seems that this approach doesn't work because several places in cxa_exception.cpp assume the exception object immediately follows field unwindHeader (there are should be no paddings at the end of cxa_exception).