Friendly ping!

Friendly ping!

Rebase

Hello @vitalybuka, I'm trying to land it on behalf of @sugak, but if I run arc patch it keeps failing with

Checking patch lib/sanitizer_common/symbolizer/scripts/global_symbols.txt...
error: lib/sanitizer_common/symbolizer/scripts/global_symbols.txt: does not exist in index
Checking patch lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh...
error: lib/sanitizer_common/symbolizer/scripts/build_symbolizer.sh: does not exist in index

Remove redundant lines from the test.

Addressed @tejohnson's comment on the test case. Thanks!

@dblaikie Thank you for the comment. I encountered this problem while manipulating LLVM IR, and not sure how realistic the code can be if I synthesize one to reproduce this problem. Still, I think this is worth fixing unless we can guarantee that debug label scope won't have a DILexicalBlockFile.

@fedor.sergeev Sorry for the late reply. I missed your comment.

Avoid -O2 from the test.

@fedor.sergeev @yamauchi I saw your discussions over llvm-dev mailing list regarding IR printing with the new pass manager, and though this might be the reason why IR printing is not supported under new PM with clang. I would appreciate if you can take a look.

@ldionne Got it. Thank you for your explanation!

I explained it over libcxx-dev mailing list, but our internal build system requires root permission when running the build and test from the remote container. I admit that this is not a common case, but still, wouldn't it be better to make the tests working under any circumstance?

@dexonsmith Thank you for your reply. Are you considering the case where -print-after-all is used along with -print-* pass? If so, shouldn't we have -print-module explicitly in the test case?

Hello, can you please explain a bit more about this change? What kind of redundant outputs avoided by this? Thanks!

@Hahnfeld Got it. Then it seems that there's not much we can do here :(

@Hahnfeld I'm not an OpenMP expert and don't have much strong argument here, but IMHO it might be confusing to have LLVM_ENABLE_FFI and LIBOMPTARGET_ENABLE_FFI have different default value from user's perspective. I wonder if we can set it off by default and explicitly turn it on for builds with test?

@grokos Thanks for the suggestion. What about this?

Addressed comments from @Hahnfeld. Thanks!

Use isReachableFromEntry. Thank you for the suggestion!

Thank you for your comments @efriedma! I addressed your comments and added a test case. The test segfaults clang without proposed change.

Friendly ping again. This is a fix for a compiler crash.

Friendly ping. Thanks!

Friendly ping for comments. Thanks!

Hello @rsmith, @wenlei and I took another look at this, and we couldn't find any use of AnonymousTagLocations outside of debug info. If that's actually the case, wouldn't it make sense to have DebugColumnInfo to control the field even if AnonymousTagLocations the part of generic printing policy?

@wmi Thank you for the concrete example! I think what we need for your example is context-sensitive profiling and function specialization, not inlining. Admittedly we don't have an infrastructure in LLVM to support context-sensitive profiling for non-inlined case and we don't perform context sensitive function specialization...

@wmi Thank you for the detailed explanation!

@wmi Thanks for taking a look! Performance numbers were neutral from our side.

Check for the new pass manager added. Thanks @wmi for the comment!

@wmi Thanks for the reply! I can totally understand that entry count is not as precise as total count, but still don't think current implementation is the right way to address the issue. As I mentioned in the summary it compares two different things (instruction level counter vs function level counter), opens up a possibility for optimizing against wrong function (e.g. long and cold function), and makes it hard to find the root cause of the performance issue.

Remove unnecessary metadata.

Addressing comments from @Hahnfeld. Thanks!

@kkwli0 Thanks for letting me know! But wouldn't https://reviews.llvm.org/D57848 always fall to "else" case added if the compiler is not GCC?

LGTM

Can you please provide a bit more context about when the client might want this function?

LGTM

Comments for the test updated. Thanks for the comment!

Friendly ping. Thanks!

@rsmith I see. Thank you for the clarification!

I think the context is Derived here. My understanding of http://wg21.link/p0968r0#2227 (in this patch's context) is that when Derived is aggregate initialized, the destructor for each element of Base is potentially invoked as well.

@rsmith @dblaikie Thank you for the comments! It seems that this is not the appropriate way to handle the issue. I'll find different way to resolve the problem.

@dblaikie I see. The problem we're experiencing is that with Clang's naming scheme we end up having different function name between the original source and the preprocessed source (as macro expansion changes the column number). This doesn't work well for me if I want to use distcc on top of our caching system, as the caching scheme expects the output to be same as far as the original source has not been changed (regardless of it's compiled directly or first preprocessed then compiled), but the distcc preprocesses the source locally then sends it to the remote build machine (and we do not turn distcc on for all workflow). I wonder if you have any suggestion to resolve this issue? Thanks!

@aprantl It is a debug info. If you compile test/CodeGenCXX/debug-info-anonymous.cpp file with clang -g2 -emit-llvm -S , you will find debug metadata something like distinct !DISubprogram(name: "foo<X::(anonymous enum at /home/twoh/llvms/llvm/tools/clang/test/CodeGenCXX/debug-info-anonymous.cpp:9:3)>", linkageName: "_Z3fooIN1XUt_EEiT_" ..., which will eventually be included in .debug_info section.

Remove conflict line.

@joerg Sorry but I'm not sure if I understand your question. This doesn't pretend to honor source code order, but makes linker to place "hot" functions under .text.hot section (There's no guarantee of ordering between functions inside .hot.text section) while "cold" functions under .text.unlikely section. This is purely for performance.

Rebase. Tests are provided in the clang counterpart (D34796).

Rebase. Sorry I somehow missed the recent comments. I addresses @davidxl's comment on documentation. Thanks!

ping!

Addressing comments from @echristo. Reverted option name change, and added a test case. Sorry I haven't work on this code for a while so it took time to invent a test case.

Hello, I observed a case where atomic builtin generates libcall when the corresponding sync builtin generates an atomic instruction (https://bugs.llvm.org/show_bug.cgi?id=38846). It seems that the alignment checking for __atomic builtins (line 759 of this patch) results the difference, and wonder if the check is actually necessary. Could anyone please shed some light on understanding this? Thanks!