lgtm

lgtm

In D73510#1843302, @MaskRay wrote:

test/CodeGen/X86/catchpad-lifetime.ll (added in rL257158 @majnemer) appears to be bit-rotted. If I delete:
...
Not test will break.

In D73505#1843138, @MaskRay wrote:

Keep a double break between function decls and defs

This patch also inserts an empty line between two declarations or two definitions.

(It actually adds a newline before printing any declaration or definition.)

Thanks!

I remember reading through this last week, but apparently I did not hit send. This patch actually prompted me to start working on the replacement for this thing.

Thanks!

lgtm

From the commit message, this sounds like it is fixing a bug. Can it be tested? Did you have a test already, or did you forget to add it?

I think I can land this version, but I'll wait until tomorrow so I'll be around to debug fallout.

• Move things around while retaining API compatibility for getParents

Some new numbers:

$ du -b build_a/bin/clang-11  build_b/bin/clang-11                                                                                                                                                                         
136375664       build_a/bin/clang-11                                                                         
141186840       build_b/bin/clang-11 

So, enabling this patch in build_b increases the size of clang with asserts enabled by 4.5MB. Odd.

lgtm :)

• three year later update, I forgot we still don't do this

lgtm

I don't think this has been a problem for mingw, GCC, & ld.bfd. At least @mstorsjo hasn't reported any issues with it. Anyway, if you can reproduce the problem, we can look into it.

lgtm

By git-phab do you mean new versions of arcanist? I don't see a git-phab tool in tree.

Code makes sense and tests look good, but I have a suggestion for how to make it simpler.

lgtm

I like the max_tokens_here / max_tokens_total variants.

I waited to see if there was any other feedback, but I'm in favor of this.

In D73139#1832396, @mstorsjo wrote:

This needs a testcase. Have a look in lld/test/COFF, it's probably most suitable with a .s test for this (the main test file itself simulating the object file created by e.g. clang, and then either a separate file in test/COFF/Inputs, or generating that second file with echo statements within the file (there's a few examples of this among the existing tests), for the other file - assemble both and try to link them together.

Looks good to me, but please let @mstorsjo confirm that this will work before landing. Thanks!

Hm, this will be an ABI compat issue that is relatively easy to run into:

typedef double __attribute__((vector_size(3*8))) MyVec;
MyVec f() { return {}; }

->
define dso_local <3 x double> @_Z1fv()
->

xorps   %xmm0, %xmm0
fldz

I posted a patch to speed things up:
https://reviews.llvm.org/D72984
The numbers are different, since 9 seconds is measured in a local linux build with no debug info, vs. 28s I measured when building at work, on Windows, with debug info.

ArmMveAliasValid is the second slowest function to compile in all of clang, according to ClangBuildAnalyzer: https://reviews.llvm.org/P8185

lgtm

lgtm, just some test comment tweaks.

I agree, raising -fgnuc-version is not an acceptable workaround, and we can't ship a clang that doesn't work with old versions of glibc. I think we need a different workaround. The simplest one I can think of is to make __warn_memset_zero_len a recognized builtin that generates no code.

In D72399#1821475, @russell.gallop wrote:

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

Is "trivially constructible/destructible" actually a requirement for this? I thought that restriction was due to host platforms which didn't support C++11 thread_local (hence if/when every platform supported C++11 thread_local then the restriction would go away).

In D72742#1822709, @emilio wrote:

GCC does avoid the zero-extension on the caller sometimes, when not required by the language and when its optimizer finds it suitable.

Didn't we work this out already when John added the alignment tracking stuff? I remember this bug involving libjpegturbo standalone assembly receiving a 32-bit argument, and then using the full 64-bit RDI register to read it, but clang stopped zero extending it. I thought the ABI doc was ambiguous at the time, and we studied GCC's behavior, and that is how we arrived at clang's current behavior. I am generally skeptical about changing behavior in this area just because the ABI doc says something now. It keeps changing. What did it say in the past? Shouldn't we pay more attention to that?

A couple of nits, but I think it looks pretty good, thanks!

I'm still in favor of this change.

I feel like this is creating too much unnecessary abstraction. I think we can use simple techniques, like unions, as I did here in this uncommitted patch:
https://reviews.llvm.org/D51664#C1373737NL442
Even better, we can do that incrementally, without many changes to the get/set value subclass data APIs.

Thanks, the code change looks good, but please adjust the tests a bit.

• rebase

In D72405#1818237, @zahen wrote:

I had no luck converting the CHECK-FOO & CHECK-NOFOO tests to use -verify because the errors were reported against "(frontend)"

• use %clang_cc1 as possible in tests

Let's get the annotations in so we can start to benefit from them.

I think I liked the first version of this patch better. I would say, if the AST after instantiation remains the same as it was before D69950, then the first one seems like the right fix. The pattern AST will just be missing a node to represent the promotion from the bitfield to a full integer, and then again to a boolean.

lgtm

I checked that __declspec(thread) and thread_local seem to be functionally equivalent with MSVC.

In D69372#1820099, @avl wrote:

In D69372#1818166, @rnk wrote:

It should be possible to use musttail thunk with a prototype of void (int, ...) between the call to foo and bar.

if I correctly understood the example - such a situation should not occur.
My understanding is that musttail thunk and it`s target _must_ have identical signatures.

Total object file size on Windows, compiling with RelWithDebInfo:

before: 4,257,448 kb
after: 2,104,963 kb

And on Linux

before: 9,225,140 kb
after: 4,387,464 kb

I'm interested in making clang do this, but I think this needs significantly more work until this is ready to land. It needs in-tree tests. I assumed we'd want to hook it up to clang-cl /Yc and /Yu.

For the noimplicitfloat mode, the compiler mustn't generate
floating-point code if it was not asked directly to do so.

Thanks, I'll rebase it and land it today. I was out skiing last week.

Honestly, MSVC's behavior makes more sense to me. Usually warnings tell the user they are doing something silly, and then let them do it anyway. Without this change, there is no way to add extra macros to some compilations, even if they won't affect the already-parsed PCH blob.

This will be a small change in behavior, but nobody on ELF should notice because things with vague linkage there are both ELF-weak and comdat.

Given that quick_exit won't work then, let's do this thing. I don't think multiple llvm_shutdown is a real use case for anyone using these APIs today.

lgtm

More reviewers: +@echristo @MaskRay