Looks good, thanks! Approved with the rename as discussed.

In D128501#3613890, @efriedma wrote:

Is there no way in LLVM to load partially-initialized memory and then freeze it that will preserve the parts of the value that are initialized? Because that seems like something that LLVM needs to be providing.

It's currently possible to load <32 x i1>, freeze the result, and bitcast it to i32. But the backend is likely to turn that into something messy. Otherwise, no, there isn't really any way to load a partially poisoned value as an i32.

Is there no way in LLVM to load partially-initialized memory and then freeze it that will preserve the parts of the value that are initialized? Because that seems like something that LLVM needs to be providing.

Thank you, LGTM.

LGTM

Seems right to me.

Okay, LGTM then

Yeah, I think the appropriate thing is to just treat the entire asm statement like it's a single full-expression. As always, that implies an annoying lifetime for blocks and C compound literals.

Minor suggestion for a cleanup, then LGTM.

LGTM. Don't worry about Swift, we'll handle it.

The cleanups arise from expressions in the constraints, right? There are a number of places where ExprWithCleanups does not mean the cleanups are independently nested within that expression; it's notably not how it works ever for something as basic as a variable initializer. If we want the lifetime of temporaries in asm operands to include the full duration of the asm statement, I think we can just declare by fiat that that's how it works for AsmStmt instead of unnaturally wrapping things this way.

Should we just diagnose this and report an error that catching pointers by non-const reference is unsupported on Itanium? We don't support it properly for class pointers anyway.

Yeah, that makes sense. Thanks for following that up.

Okay, LGTM

In D125919#3569928, @aaron.ballman wrote:

In D125919#3560523, @aaron.ballman wrote:

All that said, I think you can see why I'm hoping to get an answer from WG14 as to what to do. Reasonable folks are disagreeing on what the standard requires here.

The discussion on the WG14 reflector seems to be settling down to a consensus position that the _Atomic qualifier is only syntactically a qualifier and its use designates an entirely new type. When the standard says "unqualified type", the _Atomic is not considered a qualification. So we should *not* be stripping the _Atomic as I was doing in this patch. (SC22WG14.22200 has most of the details spelled out nicely, if you have access to the reflectors.)

This seems like the right idea to me, yeah. It doesn't look like the patch handles volatile _Atomic correctly, though.

In D126781#3565173, @junaire wrote:

I believe that OldBuilder is always valid in the normal cases, so
remove the if condition and replace it with an assertion.

I completely lost track of this, but if you'd like to rebase it, I can try to make time.

I disagree with the assessment that _Atomic behaves exactly like a qualifier. It *should* (IMHO), but it doesn't. C allows the size and alignment of an atomic type be *different* from its unqualified version, to allow for embedded locks and such. Because the size and alignment can be different, to my mind, _Atomic int and int are different types in the same way as _Complex float and float -- the object representations can (must, in the case of _Complex) be different, so they're different types. The same is not true for const, volatile, or restrict qualifiers -- those are required to have the same size and alignment as the unqualified type.

Are you now arguing that C was wrong to drop qualifiers in return types and so we shouldn't implement that rule, or...? Because your argument is much broader than just _Atomic.

Okay, I understand. So, first off, I wouldn't really call that a "weak" symbol rather than, say, a lazily-emitted symbol; "weak" already has plenty of different senses, and we should try to avoid coining more. Also, your patch description makes it sound like there's a general bug you're fixing rather than specifically an issue with re-using a single CodeGenerator to generate a succession of modules; please remember that people reading your commit messages don't necessarily know you or contextualize your patches by knowing that you work on Cling.

LGTM

Ah, true, the standard doesn't describe it as a normal qualifier. From the DR, it sounds like the committee certainly expected that this reasoning would also apply to _Atomic, even if that's not quite what they've written, but that the DR submitter seems to not want that behavior. Have you been able to track down the solicited paper mentioned in that DR? In the absence of further indication, I think dropping _Atomic like anything else is the right thing to do; like the other qualifiers, it isn't meaningful for r-values.

Ah, yeah, that seems unrelated (and surprising).

You haven't really described the issue you're having. The code looks like it's maintaining a map from symbol names to the GlobalDecl from which it'll be emitted; can you explain what Cling wants this for?

In D125919#3554195, @aaron.ballman wrote:

However, I reverted the changes in this patch in c745f2ce6c03bc6d1e59cac69cc15923d4400191 as I don't think they're correct. I've got some open questions on the WG14 reflectors regarding this function type rewriting exercise, but I think my dropping of the _Atomic qualifier is likely wrong in this patch and the fact that we're losing source fidelity in the AST is definitely an issue. The subject of https://github.com/llvm/llvm-project/issues/39595 was the behavior of the _Atomic specifier in a _Generic selection; when I realized we don't fully implement DR423, I mistakenly connected the issue with the DR. But now that I no longer think the _Atomic qualifier should be dropped as I was doing, these changes really don't address the issue in _Generic.

In D125919#3554078, @delcypher wrote:

@aaron.ballman Hey I just saw this change and had questions about it. For others looking I think the resolution to DR423 is in https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1863.pdf, I found https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_423 hard to parse.

1. It looks like we're completely dropping qualifiers on the return types of function in C. If that's the case, what's even the point of having qualifiers on return types of functions?

In D126715#3548579, @jyknight wrote:

This is rather memory-inefficient, and it's quite a shame that the ABI was defined such as not to pass-down a "context_alignment" field next to "context_size" in the async_function_pointer struct, so that the allocator can DTRT. Oh well...

Thanks, LGTM.

We definitely don't want ObjC to differ here; thanks for the CC.

I'm fine with alternatives on the names. I just don't think we want names that imply that the caller and callee are parallel here, as if there were some sort of requirement that the callee always extends. In those conventions, the callee gets passed 8 or 16 meaningful bits; if it wants an extended value, it's responsible for doing that extension, just like it would be responsible for doing the opposite extension if it wanted that.

On the one hand, I'm not sure 8M statements in a block — or 1M, for that matter — is an unreasonable implementation limit. On the other hand, this patch looks like it only changes overall memory use on 32-bit hosts, because CompoundStmt has a 32-bit field prior to its pointer-aligned trailing storage. Can you check how memory use actually changes? Ideally you'd be able to test it on a 32-bit host, but if you don't have one, just knowing the raw count of allocated CompoundStmt objects for a few large compilation benchmarks would be nice — the ideal stress test is probably something in C++ that instantiates a lot of function templates.

LGTM

I think you're right that I signed off on it; I don't remember when or where that was, or what I said about it then, but regardless I can tell you that there are two basic reasons:

• First, the result of demangling to a string is not source code. It produces valid source code as much as it can, and that's a good choice, but there are common cases where it cannot just produce source, and there is no use case that requires it to produce source. Ultimately, this is just a string that is going to be presented to a human.
• Since we're just going to present it to a human, what mainly matters is what the human's expectations are. Approximately no C++ programmers will be confused to see >> terminating a template argument list, in part because it's been a long since since C++11 was available, but also simply because it was unnatural for programmers to remember to include the space in the first place, to the point that any reasonable compiler had to catch it and recover as a common error.

It should probably be the ABI alignment in all cases; I think the preferred alignment is just supposed to be used to opportunistically over-align things like e.g. local variables, which doesn't seem relevant for the ABI code involving a call to operator new.

IIRC, the reason it works it that way is that "warnings which default to an error" are really "errors which you can explicitly downgrade to a warning". Maybe those ought to be different categories, or maybe we ought to just be telling people to downgrade this specific diagnostic instead of generally using -Wno-error.

My only comment about the design is that maybe it should be __builtin_kcfi_call_unchecked, which does not seem like something you need to hold up LKML discussion for.

Hmm. Allowing a version on -stdlib is intuitively appealing, but I'm not sure it actually gives us the information we need. As I recall, -stdlib selects the high-level stdlib and not the low-level one, and those are related in code but not necessarily at runtime; for example, you can (or at least could, historically) use libstdc++ on macOS, but the underlying low-level stdlib is going to be libc++abi, not libsupc++. And the low-level runtime is the one that actually provides global operator new functions. Is there a way to bridge that gap?

Ideally, I think, we would set this up to work something like ObjCRuntime, where we're making queries to a common place that contains all the information necessary to decide what runtime features are available. In particular, we shouldn't treat Apple platforms as forever unique in providing a stable runtime interface with availability gating.

Sounds like you need an RFC thread on the forums describing the underlying WebAssembly feature and your proposed language design for exposing it in C.

In D122963#3460976, @LiuChen3 wrote:

In D122963#3460934, @rjmccall wrote:

If people think that we'll reliably get the right behavior by the frontend adding sext and zext to call sites but not to function arguments, I can walk you through how to make that change in the frontend.

What we can't do in the frontend is just switch to unconditionally not using sext and zext at all on x86_64, because we need to maintain compatibility with old versions of clang. But we can introduce a flag that makes clang generate incompatible code if someone wants to opt in to it for performance.

Thanks. @rjmccall . I'd like to hear your approach. If the change in FE is not large, I am willing to do it. But if it is too complicated, I may not be able to do that. Firstly I found we might be able to ignore the attribute in void CodeGenModule::ConstructAttributeList in clang/lib/CodeGen/CGCall.cpp , but it is the target independent part, so I gave up.

If people think that we'll reliably get the right behavior by the frontend adding sext and zext to call sites but not to function arguments, I can walk you through how to make that change in the frontend.

From Michael Matz's comments on the GCC bug and some of the linked conversation, it sounds pretty clear that the psABI does not require extension. They could certainly be more explicit about that in the psABI, but they're under no real obligation to: the baseline assumption should always be that things not stated are not guaranteed. If the psABI intended extension to be required, the document would really need to state it positively, rather than leaving it to be inferred. Since it does not, extension is not guaranteed by the psABI.

LGTM

In D123898#3455933, @theraven wrote:

I'd like to. The test case from the original bug report was very large but I'm not sure how to trigger this with anything comprehensibly small. You need something where the callee is responsible for destroying the argument and I'm not sure what combination of properties / ABIs results in that. The original test was -(void)foo:(std::string)aString on the MSVC ABI, but I don't know how much of the Visual Studio std::string implementation is necessary to trigger this behaviour. Is it just a non-trivial destructor?

LGTM. Could you add a test case?

Sorry, that was a bit of a jumble of different ideas that I pressed into the form of a single message. Let me try to do better.

readnone etc. surely encompass the entire behavior of the call, which of course includes any callbacks it might make. And synchronization needs to be understood as effectively a store, since synchronizing with another thread which has done a store guarantees the visibility of that store on this thread; and thus readonly etc. must imply nosync. So whatever nocallback means, it must be finer-grained, suitable for situations where the stronger attributes cannot be used.

The GCC documentation really ought to specify what alignment assumptions these functions make, if any. But they control the specification here, so if they're making alignment assumptions, then I agree we probably ought to make the same assumptions.

I'm totally fine with a specifier that removes all qualifiers; I just think we can't use it for std::remove_cv.

In D116203#3434515, @cjdb wrote:

In D116203#3431612, @rjmccall wrote:

In D116203#3430332, @aaron.ballman wrote:

In D116203#3425512, @cjdb wrote:

I've noticed that libstdc++ has using __remove_cv = typename remove_cv<T>::type, which causes Clang to chuck a wobbly. Changing from KEYWORD to TYPE_TRAIT_1 didn't seem to fix anything.
Is there a way we can work around this, or should we just rename __remove_cv and friends to something else?

You could work around it by taking note that you're in a libstdc++ system header and do a special dance, but because these are in the implementation's namespace, I think it's probably kinder for everyone to just pick a different name.

I was hoping we could do something similar to struct __remove_cv which would issue a warning?

If you wanted to be especially mean, you could go with __remove_cvr, but I'd suggest __remove_cv_qualifiers instead. However, what about restrict qualifiers? We support them in C++: https://godbolt.org/z/11EPefhjf

Along with a fair number of other vendor qualifiers, yeah. I think you have to make a policy decision about whether the intent of std::remove_cv is really to just remove CV qualifiers or to produce an unqualified type (at the outermost level). The former is probably more defensible, even if it makes the transform less useful in the presence of extended qualifiers.

I'm partial to std::remove_cv being faithful to its name, unless existing implementations do something else already. I don't mind adding support for the other stuff, but if there's more than just add/remove restrict, we're going to have a combinatorial explosion for removes. Is there an alternate way we can approach this?
Possibly:

template<class T>
using remove_const_t = __remove_qualifiers(T, const);


template<class T>
using remove_reference_t = __remove_qualifiers(T, &, &&);

template<class T>
using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, &, &&); // rcv instead of cvr to prevent a typo with remove_cvref_t

In D116203#3430332, @aaron.ballman wrote:

In D116203#3425512, @cjdb wrote:

I've noticed that libstdc++ has using __remove_cv = typename remove_cv<T>::type, which causes Clang to chuck a wobbly. Changing from KEYWORD to TYPE_TRAIT_1 didn't seem to fix anything.
Is there a way we can work around this, or should we just rename __remove_cv and friends to something else?

You could work around it by taking note that you're in a libstdc++ system header and do a special dance, but because these are in the implementation's namespace, I think it's probably kinder for everyone to just pick a different name.

If you wanted to be especially mean, you could go with __remove_cvr, but I'd suggest __remove_cv_qualifiers instead. However, what about restrict qualifiers? We support them in C++: https://godbolt.org/z/11EPefhjf

In D116203#3423594, @cjdb wrote:

In D116203#3277515, @zoecarver wrote:

This patch looks awesome, Chris.

Does it make sense to have builtins for add_const, etc.? Isn't T const already kind of a builtin for this?

Potentially, but I need a core language lawyer to weigh in here. The library wording for std::add_const<T>::type is:

If T is a reference, function, or top-level const-qualified type, then type names the same type as T, otherwise T const.

Although my understanding is that the T const in this case is redundant (and thus not applied per dcl.type.cv), the library wording goes out of its way to say "do nothing".

Unfortunately, this initialization case does allow a situation where

In D122822#3420162, @rsmith wrote:

I'm concerned about the direction of this patch given @rjmccall's comments on https://reviews.llvm.org/D112626 -- presumably the way we'd want to address those comments would be to convert a __builtin_dump_struct(a, f) call into a single f("...", a.x, a.y, a.z) call in Sema, and that approach doesn't seem compatible with generating code to loop over array elements.

LGTM

Yeah. I think you could probably get away with just doing to the FramePtr argument, but it's probably safer to do it unconditionally.

It seems very likely to me that you could contrive a situation under C++ coroutines where the frame pointer is a parameter to the ramp function by the time you run coro splitting, at least under optimization. For example, https://godbolt.org/z/cGnjTbzrY, which is a reasonable implementation if you need to guarantee non-allocation and intend to assert that the size fits in the buffer. This isn't an oddity of retcon lowering, it's a general problem that emerges from the special way that the frame pointer value is assumed to be live throughout the function, which requires it to be protected against this undef'ing of arguments which would otherwise be reasonable.

In D122573#3414357, @fhahn wrote:

In D122573#3412109, @rjmccall wrote:

Hmm. We know that the big picture here, distinguishing pointers by pointee type, is going to be disruptive and will probably need a specific enabling/disabling option. I'm not sure that distinguishing only by pointer depth is a minor enough step that it shouldn't be treated the same way; in particular, it's going to start treating void * as incompatible with e.g. char **.

@rjmccall do you have any suggestions how to further reduce the initial step?

Does that actually fix anything? The problem is that the use-list of the frame pointer is being destroyed in the clones if it happens to be an argument of the ramp function. We have to stop that from happening. I don't see a solution to that other than either changing VMap or introducing an intermediate value so that it isn't just a use of an argument, and the former seems much easier.

Hmm. Doing this after doing the clones means you're only actually doing the replacement in the ramp function, which I think means it'll explode in the clones if there are uses remaining. On some level, that might be okay, because I don't think the Swift frontend ever actually uses the result of coro.begin in retcon functions except to pass it to coro.end, and retcon lowering of coro.end just ignores the argument. On the other hand, we shouldn't leave something around that'll blow up if the code pattern ever changes.

Hmm. We know that the big picture here, distinguishing pointers by pointee type, is going to be disruptive and will probably need a specific enabling/disabling option. I'm not sure that distinguishing only by pointer depth is a minor enough step that it shouldn't be treated the same way; in particular, it's going to start treating void * as incompatible with e.g. char **.

I think C++11 is pervasive enough that removing the space is fine. Demangler output is not necessarily valid source anyway.

LGTM

This is one of those patches that's difficult to review because it's so hard to foresee the consequences of changing the concepts. That said, I think the basic idea here seems reasonable.

In D120566#3346533, @arsenm wrote:

In D120566#3346506, @rjmccall wrote:

Is there something which stops you from taking the address of a kernel and then calling it? If not, are there actually any uses of kernels in the module that shouldn't be rewritten as uses of the clone?

The actual amdgpu_kernel is uncallable and has a totally different ABI, and is invoked by external driver code. From the user's device code perspective, only the callable function version is meaningful.

Is there something which stops you from taking the address of a kernel and then calling it? If not, are there actually any uses of kernels in the module that shouldn't be rewritten as uses of the clone?

It makes sense to me to do this on top of __is_trivially_relocatable, and if that returns true for more types in the future, wonderful. Obviously, it needs to be used in non-ABI-impacting ways. I'm not the right person to review this patch, though.

It's nothing to do with your patch, and you don't need to worry about it.

And this should be raised as an Itanium issue.

Changing the C++03 POD definition is going to be substantially ABI-breaking at this point. Any logic to change it needs to be platform-specific.

Sure, LGTM

If we want to assume a weaker alignment, we should change the calculation of allocationAlign rather than just changing this point downstream of that computation. But replacements of the standard operator new are restricted and cannot simply choose to return less-aligned memory; at the very least, the test program is not portable.