it is possible to list all internal keys in one place

I think @aeubanks might be a good reviewer for this.

These seem like generic substitutions that would be useful to anyone writing lit tests. We already have %{pathsep}:
https://llvm.org/docs/CommandGuide/lit.html#substitutions

Just nits

lgtm

In D110257#3134001, @JonChesterfield wrote:

So you won't articulate or document the new invariant and you think there's a llvm-dev discussion that says we can't verify the invariant which you won't reference, but means you won't add this to the verifier.

Request changes doesn't really work after you've applied the patch.

@rnk do you object to me reverting this? I don't think we can add an invariant to IR which is undocumented and unverified/unverifiable and the patch author seems opposed to fixing either omission.

lgtm

In D113707#3132812, @thakis wrote:

Fairly big update. I'd like to punt on the pragma for now since this became a lot more involved than expected already (see dependent patches; several of them have already landed).

Seems reasonable, just style nits

This roughly matches what MSVC does, where the produced object files
contain lots of temporary label symbols named like $LN<n>.

In D113027#3131371, @mstorsjo wrote:

How do you handle this with other C++ libraries, do they provide to_sys/from_sys even before C++20, or do they use some other nonstandard method, or were you only using this setup on libc++?

One alternative would be to control this setting with a pragma stack, like we do for warnings, struct packing, etc. Something like:

Let's not bring back the weak function thunk approach. That approach caused problems described in my commit, D8467, which is why the code was removed.

+@tstellar @serge-sans-paille for the recent LLVM_EXTERNAL_VISIBILITY changes
+@compnerd @MaskRay for our discussions about the shared library build

This seems reasonable, but I worry about headers that contain AT&T style inline asm blobs, such as clang's own intrinsic headers. As you say, the directive is local. I would hate to end up in a place where we have to prefix every asm blob in clang/lib/Headers/*mmintrin.h with ".att_syntax on;".

In D8467#3125386, @rjmccall wrote:

Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.

In D106585#3122726, @glandium wrote:

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

lgtm

I think the key is the self-reference in the LEA instruction:

In D112492#3119892, @tra wrote:

Yes, we do need to merge identical functions with identical names for templates.

The comdat-folding issue is different. IIUIC, it allows merging two functions with identical code and different names, into one function with two names. That will break CUDA as we do need to have each stub to have a unique address as we use it to find the matching GPU-side kernel.

I believe you have addressed John's comments, and I think the IR changes mainly affect AMDGPU users, so I don't think this will be too disruptive.

Looks good

Reviewing the code, I don't see any obvious sources of non-determinism (hash iteration). The only source I can imagine is uninitialized memory usage in the APInt. Let me know if this can be repro'd somehow.

lgtm

Sorry for the delay

Please note that before this patch, getScopeIndex would emit a LF_STRING_ID when given a DISubprogram. That's not only incorrect, but would also cause link-time error with the linker from a more recent version (Aug. 20219) of VS2019.

lgtm

Looks good from my end.

Regarding the Windows build, if it isn't reverted or fixed soon, I think the LLVM tests are sufficient. You could disable MLIR and test LLVM in isolation.

I am a bit busy at the moment.

Seems reasonable to me. +@aeubanks, can you review this? I think you ran into these issues with SystemZ extension attributes when you made that change to only look at call site ABI attributes.

lgtm

I think this seems reasonable.

So, to back up a bit, do I understand correctly that this change adds tests to the check-clang test suite that JIT compiles C++ code for the host and throws C++ exceptions? Can we reconsider that?

Please add a note to clang/docs/ReleaseNotes.rst about the behavior change.

In D8467#3095610, @zixuan-wu wrote:

Hi, I am wondering could -fsized-deallocation this be enabled by default nowadays in 2021?

Thanks for working on this, this was quite the rewrite.

In D112376#3088384, @akhuang wrote:

I think on Windows the RemoveFileOnSignal path doesn't always work when the process is killed, which causes temp files to be left sometimes (see https://reviews.llvm.org/D102736)

Aside from the cost modeling, I think there is value in having a more compact representation for this concept. It is nice to be able to turn four instructions (cast, load, icmp, assume) into one (assume). OK, maybe opaque pointers will remove the cast, but the point stands. This feature also has applications besides C++ devirtualization, so I think it's worth the added maintenance cost.

lgtm

In D112093#3074559, @mstorsjo wrote:

I don't think the default of the main COFF linker should be lowered.

But I guess one could argue whether the mingw frontend should set the default target lower - then one doesn't have to change the main default in lld/COFF/Config.h, but tweak the logic in lld/MinGW/Driver.cpp. It currently has to keep a hardcoded default version for some cases, but defaults to the COFF linker's default in some cases. We could make it always pass a version number.

What do @mati865 or @jeremyd2019 think about this?

I think this is reasonable

lgtm

In D111875#3071392, @mstorsjo wrote:

Well there's a couple uses in TempFile::keep() too. Then again, with a bit more changes, we could stop calling them too, if we aren't using the delete disposition for this file altogether.

In D112016#3070842, @aeubanks wrote:

happy to change the name if there's a better alternative

In D111875#3071350, @mstorsjo wrote:

In D111875#3071269, @rnk wrote:

Here's an idea to try to make this a bit cleaner. Maybe setDeleteDisposition should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets OF_Delete. In this case, that is TempFile::create. At that point, TempFile can retry temporary file creation without OF_Delete.

Hmm... It would indeed be cleaner in the sense that setDeleteDisposition doesn't lie about whether it succeeded or not - but that would require using a specific error code for it that the caller would look for (so we don't retry without the flag for any random error) and hope that we don't map other real system errors to that error code.

Here's an idea to try to make this a bit cleaner. Maybe setDeleteDisposition should return an error code when the path is on a network drive. That return value would be propagated back up to the caller who sets OF_Delete. In this case, that is TempFile::create. At that point, TempFile can retry temporary file creation without OF_Delete.

This flag seems reasonable. Microsoft's UnDecorateName API has quite a few of these kinds of things:
https://docs.microsoft.com/en-us/windows/win32/api/dbghelp/nf-dbghelp-undecoratesymbolname

So, after reading through Build(Unary|Bin)Op, I think the duplication of logic between TreeTransform is intentional, and the better fix is to generalize the existing logic for placeholder handling. If you enter into the Build*Op codepaths, you will re-run a lot of redundant checks. Maybe +@rjmccall can provide a second opinion.

• elaborate

There are some invariants about what family of APIs TreeTransform methods should call, and I've forgotten what they are, so I'm hesitant to approve this. Test case looks good, though.

lgtm, thanks!

Conceptually, it seems fine. As is, this will break the MSVC build, right? It doesn't support __attribute__. You need to add a portability macro in Compiler.h, or use ifdefs inline.

• fix formatting issues

Thanks for the review! I uploaded it a bit quickly because I wanted to kick off Bazel presubmit testing, since I haven't replicated that locally.

In D111195#3044594, @mstorsjo wrote:

This looks ok to me I guess. Technically in this case, this is pretty much equivalent to #ifdef _WIN32, i.e. any form when running on windows, where the process considers itself windows (cygwin doesn't define _WIN32), but I guess keeping it based on getProcessTriple() is fine too.

I'll leave it open for discussion for a while, but if there's no further opinions I can accept it later.

@rnk Do you happen to have any experience here?

Thanks for doing this, this approach looks like it incorporated my feedback on the previous approach.

It feels a little bit like a micro-optimization, but it's worth 12%.

I had another idle thought on this matter: The unsigned overflow here really isn't that weird. It's the same thing as iterating over the range [-1, 0, #args+1]. We could update all these APIs to traffic in plain signed ints, and then there would be no wrapping, just normal index math.

In D109966#3042923, @urnathan wrote:

would you prefer anonymous namespace, or stick with the static fn?

lgtm

I think I understand what's going on: these push_back instantiations are all common instantiations. They are pushing back std::unique_ptr<ZZZConceptT> objects. The emplace_back calls were all unique instantiations depending on the exact pass pointer type.

Surely this won't compile with MSVC, though?

This seems relevant:
https://twitter.com/reidkleckner/status/1198108401177251840

In D110719#3031074, @wesleywiser wrote:

In D110719#3031053, @rnk wrote:

Should I go ahead and push this for you? I will include your email in the commit Author record unless you ask me not to.

That would be great, thank you!

Should I go ahead and push this for you? I will include your email in the commit Author record unless you ask me not to.

For what it's worth, this issue is not observable from clang because clang does this mangling in the frontend. See https://godbolt.org/z/GM1Evjj3Y, it produces the function name \01_return_large@0. Looking at the code suggests that this is just an accident. It works because it is driven by the AST, which doesn't have the implicit sret parameter.

I remember looking at this code and thinking that there must be a better way to do this, but I guess it would entail making i128 legal somehow. Anyway, I don't have a suggestion ready, and this seems like a reasonable extension of the existing approach.

lgtm

lgtm, thanks!