In D64742#1606244, @glider wrote:

As a data point, Linus Torvalds suggested that we need a similar feature for GCC so that the "kernel C standard" mandates zero-initialization for locals: https://lkml.org/lkml/2019/7/28/206

Wow I thought we already deleted BBPass a while ago, good cleanup!

I think this would be useful to have in?

LGTM

LGTM: report_fatal_error() should not be considered like a "normal way" of aborting the compilation flow, crashing is never a good thing: in general I'm in favor of better error propagation (we claim that LLVM is a library framework right?)

In D63932#1679910, @ostannard wrote:

This makes the IR self-contained, which is good, but it also make the interpretation of the IR modal, which isn't great. It means that suddenly the rules of interpretation of what is valid to do or not changes according to this module flag.

I think the original version was better from that perspective - most compiler passes only need to check for one value of the attribute, and only the linker needed to care about the difference between link-unit and public visibility.

we represent the "post-link" flag in the IR (e.g. as a module flag) in order to keep the IR self-contained.

In D67262#1665677, @davezarzycki wrote:

In D67262#1665661, @mehdi_amini wrote:

I don't know what the long-term plans for this script are.

The only way for us to enforce linear history on GitHub at the moment is to mandate the use of the script unfortunately.

Food for thought:

1. git-llvm is run client side, so the linear history check isn't technically "enforced".

Works for me, but let's confirm with @thakis

I don't know what the long-term plans for this script are.

I took the responsibility of reverting in r371480, since I approved this revision without anticipating the behavior change.

Do we have the same ability as git push to specify the remote? The equivalent of git push origin master (which works without having an upstream branch)?

LGTM.
Nice! I didn't this notation, apparently it's been there since 1.7.
You can also use @{u} shortcut for it, I'll try to remember this

Seems like an attribute that could go on ArrayRef as well?

with a funny command line like clang -Oz -flto -o foo a.o b.c, where one (or all) of the input arguments is a source file? Would it invoke the bitcode compile of the source files with the requested -Oz and then invoke the LTO linker plugin with -O2/-O3?

I assume I might be missing something here, though, since someone mentioned this above (I don't understand the response to it though).

In D65774#1616200, @mehdi_amini wrote:

Isn't clang warning in the other direction? (incompatible with GCC)

Isn't clang warning in the other direction? (incompatible with GCC)

• Improve output when asking the user a question
• simplify python 2/3 compatibility

@jyknight : ping?

In D65295#1602763, @beanz wrote:

The CMake build system option PLUGIN_TOOL on add_llvm_loadable_module is for this specific purpose, it instructs the link step for the plugin dylib to resolve symbols against the tool.

Polly likely needs to generate different libraries for being a clang plugin vs being an opt plugin, but that is expected and the correct solution to this problem.

unfortunately does not solve the more general problem that not all symbols are available to plugins in statically linked tools. I am open for better solutions.

default to No

Change abort message to "Aborting"

Fix python 2/3 compatibility

The motivation is unclear to me at the moment? Why is it a good thing?

LGTM (after you remove the typo)

In D63976#1566236, @ruiu wrote:

I agree with Teresa. I don't think automatically setting O3 for Os and Oz is a good idea because these three options are different (that's why we have three different options in the first place). Adding an Os and Oz to lld's LTO option seems like a good idea, but they should be mapped to their corresponding features.

In D60495#1563127, @tejohnson wrote:

In D60495#1563124, @mehdi_amini wrote:

LGTM, but it'd be nice if @tejohnson or @pcc would also approve.

I added a suggestion a couple hours ago, just for commoning the code between the two LTO APIs.

LGTM, but it'd be nice if @tejohnson or @pcc would also approve.

Sorry I missed the previous updates.

LGTM

LGTM, thanks!

(Patch is missing context)

Wow that's much simpler!

LGTM.

It is good idea, thanks for putting this together!

Looks great! Thanks.

Thanks!

In D58391#1408322, @tejohnson wrote:

In D58391#1403188, @efriedma wrote:

What's the general state of the LTO pipeline at this point? PassManagerBuilder::addLTOOptimizationPasses is adding passes in a really weird order (in particular, the placement of the inliner is really strange). Has anyone looked at it recently? Would it be worth killing it off in favor of the ThinLTO pipeline just so we don't have to maintain it?

The reason they don't use the same pipeline is that it would be too expensive for regular LTO mode which is serial. ThinLTO can use a more aggressive post-link pipeline due to the parallelism. Mehdi did some measurements a couple years ago and found that using the ThinLTO pipeline for regular LTO increases the compile time in that mode significantly.

LGTM, but you should probably wait to hear from someone else as well

In D58384#1402418, @evgeny777 wrote:

Summary clusters, nodes and edges are retrieved from DenseMap with arbitrary order, which doesn't make result DOT file invalid.
Reverse iteration just exposes test issues.

Is it expected that the test output is sensitive to reverse iteration?
It sounds to me more like an issue with the code than with how the test is expressed.

In D58157#1397302, @rnk wrote:

In D58157#1397274, @mehdi_amini wrote:

It'd still be nice to identify the end goal with most cfe people (i.e. even if you land this right now, discussing the desired layout in the monorepo)

I sent off http://lists.llvm.org/pipermail/cfe-dev/2019-February/061290.html, so the larger community is aware of this change. I don't have much to add to a larger discussion about possible code reorganizations, but I suppose folks may come forward to express their opinions.

In D58157#1396824, @rnk wrote:

And, this change really just keeps us at parity with what we had with svn. We can always revisit the decision to merge the clang tools into clang. This particular change just gives us more options, today.

In D58157#1396072, @thakis wrote:

In D58157#1395762, @mehdi_amini wrote:

In D58157#1395716, @rnk wrote:

I think we have consensus,

Based on three comments in a revision? Seems strange to me.
I don't really care about this, so do whatever you want, but I would expect that "consensus" means an actual wider discussion (i.e. llvm-dev + cfe-dev).

Please cite said discussion for when you added this, as requested above.

In D58157#1395716, @rnk wrote:

I think we have consensus,

What is the motivation to change this now?

Could we sanitize the list of LLVM_ENABLE_PROJECTS and error for unknown entries?

@dexonsmith: I'm not sure why a new differential should be created? I updated reviewers in this one.