In D53596#1298787, @dexonsmith wrote:

Sorry, I missed your questions. Thanks for pinging.

I wasn’t concerned about this patch going in. I was just concerned we may have had compile time regressions already. Metadata forward references are quite expensive to track and resolve.

But I think I’d misread the patch entirely. Is see this is just loosening to Metadata .

Sorry, I missed your questions. Thanks for pinging.

In D53596#1287633, @dexonsmith wrote:

Hmm. I remember writing this code. I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.

Hmm. I remember writing this code. I have a faint recollection of proving at the time that these could never be forward refs, and that it was important for bitcode reading performance.

LGTM.

In D53796#1283055, @ldionne wrote:

I don't feel like I'm qualified to review this change. Adding Duncan who knows more about linkers. I'll avidly watch to learn though :-)

This approach seems reasonable, but it would be great to have a chance to run some numbers internally to be sure we not losing something important.

This LGTM with a couple of nitpicks.

In D50539#1273654, @vsapsai wrote:

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

In D41474#1270703, @tejohnson wrote:

In D41474#1270284, @hiraditya wrote:

Can we land this patch

Let me take a look today. As noted in my earlier comment, this still seems like a hammer to fix something that was intended to be handled earlier by the MDLoader. I'd like to understand why it didn't work as intended.

This is awesome.

In D53260#1265712, @lhames wrote:

Committed as r344542.

In D53260#1265686, @dblaikie wrote:

Looks good to me (I guess the capacity thing there could be tested too - ensuring that it didn't roll up to a larger capacity than intended?)

Unfortunately DenseSet / DenseMap do not expose a capacity function at the moment. I believe it's just getNumBuckets, so unless there is some reason not to I would be in favor of adding a public 'capacity' function that returns that. Then we could add that check to the unit test.

Thanks for working on this. However, it seems to be introducing the same sorts of problems (dropping vtable anchors) as the revision you've linked to. We should be adding vtable anchors, not removing them.

In D52863#1256253, @rtrieu wrote:

https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers

Well, learned something new today. I'll follow up with something that doesn't mess with the vtable anchors.

We shouldn't be dropping vtable anchors like this.

The change looks correct, but I'm not sure it's the right one. Looking at Instructions.h at getCalledFunction():

/// Return the function called, or null if this is an
/// indirect function invocation.
///  
Function *getCalledFunction() const {
  return dyn_cast<Function>(Op<-3>());
}

it seems like we could just change this to dyn_cast_or_null, and that would also match the documentation comment.

In D52312#1241110, @fhahn wrote:

In D52312#1240764, @dexonsmith wrote:

I wonder, could these be replaced by TinyPtrVector? They're only 8B.

Interesting, I'll have a look. My only worry with this is that in the LSR and SLP use cases, it is very likely that the vector contains more than 1 element.

I'm concerned about propagating this pattern. DenseSet, like DenseMap, is tuned for small keys and values. SmallVector<void*,4> is not exactly small, at 48B. And this patch makes it easy to throw much larger SmallVectors into these containers.

LGTM too.

LGTM.

+cfe-commits
-llvm-commits

This is awesome to see!

In D51440#1225318, @steven_wu wrote:

I do prefer the current approach especially on Darwin. Some concerns of switching to use "-L + -l" are:

1. clang and compiler-rt are rev-locked. Inferring the compiler-rt from resource-dir and passing to linker with the full path can prevent mistakes of mixing them up.
2. This change does change linker semantics on Darwin. ld64 treats the inputs from command line with highest priority. Currently ld64 will use compiler-rt symbols before any other libraries passed in with -l flag. If clang decide to pass compiler-rt with -l flag, any other libraries (static or dynamic) that passed with -l flag will takes the priority over compiler-rt. This can lead to unexpected behavior.

In D51497#1220621, @sgraenitz wrote:

In D51497#1219835, @dexonsmith wrote:

You should check in LLVM textual IR files (.ll) instead of C++ source files or object files. Using clang -S -emit-llvm (or llvm-dis some-thinlto-bitcode-file.o) should get you started.

Yes that would be great, but wouldn't it require IR representation for summaries? I didn't double-check myself, but there's a note in the language ref, "that temporarily the summary entries are skipped when parsing the assembly". Not sure if I understand that correctly, but I was under the impression that we can't have summaries in IR files yet.
https://llvm.org/docs/LangRef.html#thinlto-summary

In D51497#1219698, @sgraenitz wrote:

Not sure what's the best way to get rid of the binary object files. IIUC the ThinLTO tests are lit-based, so they build their summary-enhanced objects with clang and test via llvm-lto2 tool. Maybe I can turn all the unit tests into lit-test and add a hook to llvm-lto2? Don't know if there is a way to emit object files with summaries using yaml2obj or so? Any ideas?

In D30009#1218647, @rsmith wrote:

One consequence of this patch (see https://reviews.llvm.org/rL341002) is that adding documentation to an existing attribute changes the behavior of Clang. That does not seem acceptable to me: documentation improvements should not change which attributes we allow this pragma to appertain to.

If we really want an "only documented attribtues can use this feature" rule (and I'm really not convinced that is a useful rule to enforce

In D51240#1213358, @davidxl wrote:

Re "Why not":

The common use model of instrumentation based PGO and SamplePGO are quite different. The former usually uses 'fresh' profile that matches the source. If there are massive code refactoring or ABI changes, the user can simply regenerate the profile. For the latter, the profile data is usually collected from production binaries, so the source is almost always newer than the profile.

In D51240#1213179, @davidxl wrote:

In particular, I don't see much need to do this for instrumentation based PGO [...]

LGTM if thakis is happy.

In D50719#1199427, @mclow.lists wrote:

I have pissed in this area, too - See https://bugs.llvm.org/show_bug.cgi?id=38495 and the proposed resolution here: https://bugs.llvm.org/attachment.cgi?id=20692
How about I just make this change as part of that fix?

In D50599#1199411, @erik.pilkington wrote:

In D50599#1199289, @labath wrote:

I have one process question though. What's the current workflow for merging changes between the demangler copies in libc++abi and llvm? Right now they're mostly identical except for a few macros, but CRTP will require putting a lot of stuff into the header (and into a namespace), so they will diverge more. Do you think it would make sense to factor the common bits into a separate (.inc ?) file which can be kept identical? Then cxa_demangle.cpp could just #include that file into an anonymous namespace and llvm could include that into the llvm namespace (from a header file).

I have an ugly perl script that I use to patch changes from libcxxabi to llvm (see the end of this message). The basic idea for compatibility in libcxxabi is ItaniumDemangle.cpp maps to libcxxabi/src/cxa_demangle, and the other headers in llvm/lib/Demangle/* map to their equivalents in libcxxabi/src/demangle/*. I would like to keep them identical to keep patching easy, so any changes to the file structure in llvm should also be done in libcxxabi.
I think that we should move the Node hierarchy into it's own file ItaniumNode.h, and the CRTP parser into one, ItaniumParser.h which would include ItaniumNode.h, and put all the declarations in the namespace llvm::itanium_demangler (or an anon namespace in libcxxabi). That would allow us to implement ItaniumPartialDemangler in a separate file (which wouldn't map to anything in libcxxabi), which would be nice. ItaniumDemangle.cpp could just be a #include "ItaniumParser.h" and the implementation of itaniumDemangle(), or __cxa_demangle in libcxxabi. Does that sounds reasonable? CCing @rsmith, who expressed some interest in moving the AST/parser to a header, and @dexonsmith, who's had some thoughts on this before.

This looks correct to me, but I wouldn't mind having someone else take a look too.

Hi Peter,

In D49240#1177386, @ldionne wrote:

In D49240#1177382, @EricWF wrote:

LGTM. Assuming this doesn't break the ABI list checks.

I talked to @ldionne about if we wanted to land this right before 7.0, but I think we can. If we have to disable the internal linkage change, we can at least keep the new naming scheme and start transitioning to it.

I'd like to get input from @mclow.lists and @dexonsmith about whether we should move forward with this before we branch 7.0, or wait after the release. This is a wide reaching change so I want to share responsibility ;-)

Committed in r338071.

Nice cleanup! LGTM.

Committed in r337820.

FWIW this LGTM, but since Marshall already commented you'll need his sign-off as well.

LGTM.

LGTM too.

In D48518#1169148, @dexonsmith wrote:

In D48518#1169138, @dexonsmith wrote:

Reverted in r337511 due to a bot failure:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/26526

Hmm... that bot was green again in r337508, before I had a chance to revert. Maybe the blame-list was faulty.

In D48518#1169138, @dexonsmith wrote:

Reverted in r337511 due to a bot failure:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/26526

Reverted in r337511 due to a bot failure:

#8 0x000055581f2895d8 (/b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/bin/clang-7+0x1eb45d8)
#9 0x000055581f294323 llvm::ConstantAggrKeyType<llvm::ConstantArray>::create(llvm::ArrayType*) const /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/ConstantsContext.h:409:0
#10 0x000055581f294323 llvm::ConstantUniqueMap<llvm::ConstantArray>::create(llvm::ArrayType*, llvm::ConstantAggrKeyType<llvm::ConstantArray>, std::pair<unsigned int, std::pair<llvm::ArrayType*, llvm::ConstantAggrKeyType<llvm::ConstantArray> > >&) /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/ConstantsContext.h:635:0
#11 0x000055581f294323 llvm::ConstantUniqueMap<llvm::ConstantArray>::getOrCreate(llvm::ArrayType*, llvm::ConstantAggrKeyType<llvm::ConstantArray>) /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/ConstantsContext.h:654:0
#12 0x000055581f2944cb llvm::ConstantArray::get(llvm::ArrayType*, llvm::ArrayRef<llvm::Constant*>) /b/sanitizer-x86_64-linux-autoconf/build/llvm/lib/IR/Constants.cpp:964:0
#13 0x000055581fa27e19 llvm::SmallVectorBase::size() const /b/sanitizer-x86_64-linux-autoconf/build/llvm/include/llvm/ADT/SmallVector.h:53:0
#14 0x000055581fa27e19 llvm::SmallVectorImpl<llvm::Constant*>::resize(unsigned long) /b/sanitizer-x86_64-linux-autoconf/build/llvm/include/llvm/ADT/SmallVector.h:347:0
#15 0x000055581fa27e19 (anonymous namespace)::EmitArrayConstant(clang::CodeGen::CodeGenModule&, clang::ConstantArrayType const*, llvm::Type*, unsigned int, llvm::SmallVectorImpl<llvm::Constant*>&, llvm::Constant*) /b/sanitizer-x86_64-linux-autoconf/build/llvm/tools/clang/lib/CodeGen/CGExprConstant.cpp:669:0

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/26526

In D49532#1169018, @echristo wrote:

In D49532#1169016, @dblaikie wrote:

Probably squirrel it away in the generic 'flags' bucket?

Seems reasonable to me. Might need a tristate or something, but that's what enums and masking are for.

Committed in r337504.

Shouldn't this have -print-module in the command-line, since that's what this is supposed to be testing? I must have missed that in the original review.

Committed in r336869!

Ping!

In D48991#1156565, @dblaikie wrote:

Only lambdas, and not other functors (like std::function, etc)?

So we could end up with code like:

MyFunctor = myLambda;

Though I suppose that's OK - and it's only in the local scope of a function
& not like any interesting object-like operations (such as assignment) can
be done with the lambda as the subject anyway, so it does look a lot like a
function... (except you can't assign it to a function pointer - but at
least the lambda should be nearby in the code so that's not too confusing)

Thanks! (I'm not comfortable signing off though, since I don't know much about dllimport.)

Is it impossible to also test this locally (in the LLVM repo)? It will be hard to hack on this file if there aren't local tests.

LGTM too.

LGTM.

In D49030#1155065, @labrinea wrote:

In D49030#1155000, @chandlerc wrote:

We already have SetVector which is widely used for these patterns. If we need both, we need a clear explanation of the difference and why we need both (IE, why users of one can't use the other).

The remove operation on a SetVector can cost linear time and this might be an issue in some cases. Please have a look at the review comments of https://reviews.llvm.org/D48372

In D48857#1153613, @ruiu wrote:

It may make sense to get a consensus and update the coding standard
document, as it is not entirely clear. If that's the case, I'd vote for
CamelCase. :)

I've started an RFC on llvm-dev to discuss whether we should make this change:
http://lists.llvm.org/pipermail/llvm-dev/2018-July/124466.html

In D48857#1153575, @ruiu wrote:

Is that part of our coding standard? In lld, all local variables holding
function-like objects are in CamelCase, not in camelCase.

In D48723#1148297, @tyb0807 wrote:

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

In D48892#1153473, @davide wrote:

The lldb bot started failing very recently and the blamelist hints at this change.

http://green.lab.llvm.org/green/job/lldb-cmake/7777/

Can you please take a look?

For your convenience, this is failing building LibCxx testcases with a linker error:

Build Command Output:
Undefined symbols for architecture x86_64:
  "std::__1::__vector_base_common<true>::__vector_base_common()", referenced from:
      std::__1::__vector_base<int, std::__1::allocator<int> >::__vector_base() in main.o
      std::__1::__vector_base<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__vector_base() in main.o
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [a.out] Error 1

In D48983#1153466, @ldionne wrote:

It's pretty obvious that this is the problem when looking at https://reviews.llvm.org/D48892 (grep for __pbump), but TBH I haven't been able to run the check-cxx-abilist test locally. Mine fails with like 500 differences, so there's got to be something I'm doing wrong.

LGTM.

LGTM.

Awesome. I wonder, would it be worth measuring compile-time impact for optimized builds?