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.

Thanks for fixing this!

In D57422#1376600, @pcc wrote:

In D57422#1376550, @sbc100 wrote:

Can I ask why you don't want --lto-O0 to set CGOptLevel to None?

The brief answer is: --lto-O0 means no cross-module optimizations,

You can avoid the git status pollution by adding the build directory to .git/info/exclude.

In D57330#1375096, @labath wrote:

This is not an full out-of-source build, since the build folder is still a subfolder of the repo root

LGTM, but for one comment that requires a fix I believe (lld on Windows)

In D56485#1363031, @filcab wrote:

@pcc: Since this impacts CFI, can you say if the perf impact is acceptable on your end?
@mehdi_amini: Same Qs, but about macOS

LGTM.

In D47073#1360077, @jfb wrote:

That's incorrect. I don't care what the policy is as long as it's sane. 3 years was proposed on the RFC, we went with it. Then nuance was asked for on this review, we went with it. Then it was asked that time was made a soft constraint, we went with it. Then more bikeshed happened.

I can't add much more than what @jyknight wrote here: https://reviews.llvm.org/D47073#1358937 ; this summarize perfectly my view of the situation.

I don't want to give the impression I'm stalling this just for the sake of being difficult, and since it is easier to talk "with code" I quickly wrote this: https://reviews.llvm.org/D56770 . This *not* a patch intended to be submitted, it is just a stripped-down version of how I'd phrase/structure it intended to support my arguments in this thread. Note that I would intentionally left out any compiler minimum bump out of the policy upgrade change, as these are conceptually separate change anyway.

In D47073#1358663, @jfb wrote:

s/two/one/ : I've talked to Chandler already, and he was leaning on others (in large parts, me) to help Erich with the review

In D47073#1358640, @jfb wrote:

In D47073#1358626, @mehdi_amini wrote:

In D47073#1358593, @jfb wrote:

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

I strongly disagree with this right now: this is *not* OK to rush this as is.

If you need something in the release, then it can be done in a much less controversial way.

What makes you say this is being rushed?

In D47073#1358593, @jfb wrote:

OK I think we've had more than enough bikeshed, over months, and in-person at the LLVM dev meeting. This is good enough, we want to warn people for the LLVM 8 branch which is tomorrow. Let's get this checked in today. We'll start a discussion on LLVM dev when anything *actually* changes, so nobody needs to panic just yet.

Also the two levels (warnings vs errors) is not appropriate to accomplish what you're describing.

I don't understand what you're asking to change here

One release cycle with deprecating the version we're about to remove support for using a discardable cmake error seems like a good idea in general: we should encode this in the policy as well.

In D47073#1358067, @jfb wrote:

In D47073#1358066, @mehdi_amini wrote:

In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

The current version of the patch would issue a fatal error when using a host clang 3.5, not just a warning
(note that clang-3.4 is supposed to support C++14)

Yes it's a CMake error. The point is: you won't see it and will ignore it if it's just a CMake warning. What I meant was: we're giving everyone with older compilers a warning that they won't be supported soon. They can opt out and compile this once. Won't be true soon (because we'll move away from C++11). I want them to know ASAP.

In D47073#1358039, @jfb wrote:

Yes: we want to warn folks early. This is just warning them, and they can opt-out.

In D47073#1354798, @zturner wrote:

In D47073#1354779, @mehdi_amini wrote:

I'd like to see docs/GettingStarted.rst updated to include some language from what Chandler mentioned. In particular upgrading a toolchain has to be *motivated* by explicit benefits, and the toolchains dropped must be evaluated with respect to the general availability for the users.

I think this is implied [...]

I'd like to see docs/GettingStarted.rst updated to include some language from what Chandler mentioned. In particular upgrading a toolchain has to be *motivated* by explicit benefits, and the toolchains dropped must be evaluated with respect to the general availability for the users.

What about creating a new macrothat carry the semantic we're looking for here: "LLVM_KEEP_FOR_DEBUG"?
We can conditionally define it based on NDEBUG or add a CMake flag for it that is enabled in debug build (the latter has the advantage that it would allow to turn it on even in release builds when needing to debug these)

In D54877#1309017, @zturner wrote:

In D54877#1308855, @zturner wrote:

at the end of the day, comments that arise in code review should take precedence over what the style guide does or doesn't say when it concerns readability, since it's an actual account of someone stating that they do or don't find something readable, and that's what we're trying to optimize for.

In D35035#1253023, @spatel wrote:

That case (and a couple of similar tests that I tried) are handled by -sroa, so they probably never made it to instcombine in the 1st place. I don't know anything about SROA, but hopefully, it's making that transform using some principled logic. :)

(should fix PR39780 https://bugs.llvm.org/show_bug.cgi?id=39780 )

If we go with this document moving forward, I would first purge it so that it reflects the current situation. Otherwise we could also just archive it and start a new one with the actual action plan.
I feel adding more stuff here will be a bit confusing moving forward.

@kristina both bfd and lld are adding the .a extension when you pass a lib through -l. Similarly to -llibfoo won't work, -lfoo.a will lead the linker to look for libfoo.a.a

LGTM

LGTM

I'm worried, however, about generating a bunch more code than needed from clang in the hopes that the compiler will clean it up later.

In D49771#1183380, @jfb wrote:

In D49771#1181008, @mehdi_amini wrote:

I'm curious: isn't the kind of optimization we should expect LLVM to provide?

Maybe? It seems obvious to do here since we know we'll probably want to be doing it, and I have another patch I'm working on which will make it that much more obviously useful to have here. The middle-end can definitely figure it out but it just seems like more work, later, so in the meantime we'd be looking at more stuff.

I'm curious: isn't the kind of optimization we should expect LLVM to provide?

but declarations cannot be internal.

LGTM.

std::map is usually fairly slow, have you considered sorting at the time you write to the file instead? That would add a slow step when writing the files but wouldn't affect any in-memory operation.

In D43253#1011978, @hans wrote:

In D43253#1010580, @mehdi_amini wrote:

In D43253#1010451, @qcolombet wrote:

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

Note: we need buy-in from LLVM 6.0 release manager (@hans ?) that this will be cherry-picked there before landing this in trunk.

This seems fine for 6.0 (it needs to land soon though), but why does it need to be cherry-picked to the branch before landing on trunk?

In D43253#1010451, @qcolombet wrote:

Thanks all for the great feedbacks!

The final solution is much better than a dumb bump of the bitcode version.

LGTM.

@hans : this needs to go in LLVM 6.0, is it OK?

In D43253#1009298, @mcberg2017 wrote:

Strongly disagree. The new test's purpose is to provide functional testing on the new flags in the new format. Sanjay, now would be a good time to chime in.

In D43253#1009292, @mcberg2017 wrote:

Case in point:

./bin/llvm-dis < /Users/michael_c_berg/llvm/llvm/test/Bitcode/compatibility-3.6.ll.bc | ./bin/FileCheck ../llvm/test/Bitcode/compatibility-3.6.ll
../llvm/test/Bitcode/compatibility-3.6.ll:617:11: error: expected string not found in input

; CHECK: %f.fast = fadd reassoc nnan ninf nsz arcp float %op1, %op2
         ^

<stdin>:445:2: note: scanning from here

%f.fast = fadd fast float %op1, %op2

in this case we are using my current patch with module version at 2 in the key .bc file, however we need the mapping as:

reassoc
nnan
ninf
nsz
arch
contract
afn

the new format writes reassoc in position 0 and aliases to UnsafeAlgebra incorrectly in this case, causing us to map fast, when less than fast was specified in the IR.
This will still be the case with the other format.

^