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.

^

In D43253#1009275, @mcberg2017 wrote:

So the compatibility .bc files in check test have usages of the current flag layout with reassoc, afn and the new layout, as has been the case since last Nov. These tests are not old fast format but sub flags tests in the new format, and require the new mapping.

@tstellar YES.

In D43253#1007097, @mehdi_amini wrote:

In D43253#1006726, @mcberg2017 wrote:

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

Can you elaborate what you mean by "they require the current fast sub flags format for part of their use"?

I quickly put a patch to change the encoding, may be easier to discuss with code at hand: https://reviews.llvm.org/D43348
(I didn't try to build it, just a quick draft)

Actually if we're making this change, we consider that bitcode generated between the introduction of the new fast-math flags and now isn't supported anymore.
If so then we could instead of bumping a version, change the encoding of the FMF record (or create a new record type), so that the old encoding is upgraded but not the new one.
That would seem to me to be more in line with the usual handling of such situation in the bitcode.

In D43253#1006726, @mcberg2017 wrote:

For the 6 bitcode files, they are special context, as they require the current fast sub flags format for part of their use, so we have to bring them up into the current context as they require the use of the new flags selectively with the bumped version number.

In D43253#1006664, @mcberg2017 wrote:

Uploaded above are the 6 bitcode files we will need to replace for check to run successfully as these files had the old version number in them.

In D41297#974984, @tejohnson wrote:

In D41297#974946, @mehdi_amini wrote:

In D41297#974754, @evgeny777 wrote:

At which point of the API is this guarantee?

1. ThinLTO.ModuleMap stores BitcodeModule objects and uses them during entire thin link phase (until backend threads are launched)
2. BitcodeModule contains pointers to BC data, so this data should be actual for module to be parsed)
3. Module summary is exported to .dot in CombinedIndexHook which is invoked in the beginning of thin link phase

None of these seems like API guarantee to me, it sounds like "this is what actually happens in practice today in this particular workflow".

It seems it must be a guarantee of the LTO API, since ThinLTO.ModuleMap holds the BitcodeModule objects and accesses their buffers through and after the thin link.

In D41297#974754, @evgeny777 wrote:

At which point of the API is this guarantee?

1. ThinLTO.ModuleMap stores BitcodeModule objects and uses them during entire thin link phase (until backend threads are launched)
2. BitcodeModule contains pointers to BC data, so this data should be actual for module to be parsed)
3. Module summary is exported to .dot in CombinedIndexHook which is invoked in the beginning of thin link phase

LGTM

The test still seems complex to me (do you need all the IR types? The code in the functions? And all these metadatas and metadata fields?

In D41296#962279, @serge-sans-paille wrote:

In D41296#962215, @hfinkel wrote:

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better.

I thought of this, very happy you suggest we go that way. In a release build, we could activate that by default and keep the names in debug build.

In D41297#958339, @evgeny777 wrote:

@mehdi_amini I've updated diff with one which no longer requires post-processing at the cost of having extra StringRef in GlobalValueSummaryInfo

add notation about the importing decision, promotion, dead-elimination, ... on every edge/nodes.

It this supposed to be done as comments in DOT file?

In D41267#957510, @vitalybuka wrote:

I agree that implicit contract about streams order is not nice here, and we should consider to make it explicit.

In D41267#957237, @vitalybuka wrote:

In D41267#957200, @mehdi_amini wrote:

However small changes in input, could trigger combined module and shuffle outputs making life of llvm::LTO harder.

Sorry but this is still loaded in assumption about the client, which seems dubious in general to me.

At this point I am trying to solve problem is the gold plugin, however I'd expect this could be helpful for other clients as well.
Anyway why benefit of particular client without regressing behavior for the rest is a problem? Especially considering that the patch simplify implementation.

However small changes in input, could trigger combined module and shuffle outputs making life of llvm::LTO harder.

I suspect it should be possible to do this after the ThinLink and add notation about the importing decision, promotion, dead-elimination, ... on every edge/nodes.

Neat!

This patch doesn't seem motivated, the only effect I can see is that a ThinLTO link that don't include any LTO module would create a module while it was lazily created only if needed before.

The commit message does not make much sense to me in the context of this C++ API: there is a non-negligible layer between the build and the build system rules and the LTO implementation, i.e. the linker.
Implementing a contract that makes it convenient for your build system in the implementation of the LTO "engine" behind this API looks like leaking abstractions to me.

I wrote this code in April 2016 ( http://reviews.llvm.org/D19103 ), in the very first approach to internalization, which was inferred in the backend and wasn't bullet proof (especially with respect to assembly by the way).
Since then, we moved all decision in the thin-link and this code is indeed (hopefully) dead.

LGTM.
Did you check clang?

LGTM

Was this done using clang-tidy (or similar tool) or manually?

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

In D38154#886712, @chandlerc wrote:

In D38154#886708, @mzolotukhin wrote:

I'd recommend using LNT,

This seems like a really cumbersome process to expect folks to go through for changes intending to improve compile time. Is there no way to simplify it?

In D38154#886504, @chandlerc wrote:

Mehdi, would you feel comfortable with this as-is?

In D38298#883561, @ilya-biryukov wrote:

And it would be nice to have this change right now as a quick workaround.

In D38298#883177, @klimek wrote:

In D38298#883091, @mehdi_amini wrote:

In D38298#883025, @klimek wrote:

"make install" is not primarily a distribution mechanism, docker is (most folks upload their docker images to public registries without thinking much about this).

I'm not sure why "primarily a distribution mechanism matters? How is one suppose to distribute?

docker push to a public registry?

In D38298#883025, @klimek wrote:

"make install" is not primarily a distribution mechanism, docker is (most folks upload their docker images to public registries without thinking much about this).

Ideally, I think all the headers would hide the method behind #ifdef LLVM_ENABLE_DUMP , so that it wouldn't be a linker failure but a compile time failure!

Thanks for fixing this :)

In D38298#881699, @ilya-biryukov wrote:

In D38298#881628, @mehdi_amini wrote:

Can you elaborate why this script is desirable/needed?

Sure, sorry for leaving out the context.

Including licences is technically required in any binary distribution of LLVM, including ones inside Docker images.
Copying the licenses could be made optional, but always putting the licenses into Docker images should not hurt either.

Can you elaborate why this script is desirable/needed?

Did you check ld64 in Xcode 9? I think ld64 was fixed to append the suffix as well.

LGTM

LGTM otherwise.

I don't mind where you install it, but we can also assume that someone who use this Dockerfile intends to use clang, so why not adding it to to update-alternatives as well, and even making it default for /usr/bin/c++ and /usr/bin/cc?

I'd like to also see a testcase for the situation where we trigger the emission of a declaration with an available_externally definition and then find we need to promote it to a "full" definition:

In D37099#852298, @ilya-biryukov wrote:

In D37099#852251, @mehdi_amini wrote:

Would this be all obsolete with git repos?

I would think so.

Also all the python code seems complicated to me for what is basically find | grep -v '/\.git/' | grep -v '/\.svn/' | LC_ALL=C sort | tar -cf - -T - --no-recursion | sha1sum, can you elaborate why this is needed?

That's roughly what I started with, but's it's more complicated if you want to compute separate checksums for projects inside single tree source checkout (i.e. llvm, llvm/tool/clang, llvm/projects/lldb, etc.).

Would this be all obsolete with git repos?

Bi-weekly ping! (@rsmith)

LGTM

Seems fine to me. Adding @dexonsmith (I don't know who maintain libc++ at Apple right now)

Is this something that is affecting clang-5.0 backward compatibility with clang-4.0?
(adding @hansw for visibility)

LGTM.

Address Richard's comment

I'll leave the final approval to Teresa.