Clang is trying to avoid using non-byte-multiple integer types for memory accesses in LLVM IR, whereas Swift isn't. If you take your test case and make the field non-atomic, you'll see that: Clang is accessing the field as an i8 and Swift is accessing it as an i1. So there's a deeper mismatch here which in practice works out because there's no actual difference in memory access width between those types.

Hmm. The way you'd test it on the Clang side would be to just write a test case that passes such an aggregate and tests that it's passed as an i8. But I'm not sure it's at all unreasonable to pass this as an i1 rather than an i8; seems like something that Swift should handle correctly in its call-lowering code.

I'm sorry that it's taking a while to find time to review the implementation; I'll try to get to it this week.

Yeah, that's definitely an LLVM test.

Okay, LGTM.

In D89490#2483792, @aguinet wrote:

In D89490#2482531, @rjmccall wrote:

I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few developers.

I am not saying this exact feature is needed by Wine. What I am doing is a parallel with __attribute__((ms_abi)), which is exactly the same as this exact feature, but to target the MS ABI from a foreign OS (instead of the Darwin one). Wine just can't work without that attribute.

Let's imagine that, at the time people had wanted to introduce __attribute__((ms_abi)), we would have told them "this is a niche, hack <insert compiler representation> instead". Do you really think that would have been a serious solution?

I'd say that we are in a chicken and egg problem: we don't have users for this, but maybe because we don't have the feature. At my company, we already have internal tools that are using this feature, and have lots of hopes it will simplify some of our more complex internal CI and debugging processes. We plan to open source all of this, because we also believe that will also help others with the same kind of issues. But I agree that we might be wrong, I honestly don't know.

LGTM

I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few developers.

In D93377#2481957, @hubert.reinterpretcast wrote:

In D93377#2481312, @rjmccall wrote:

Are you committed to the name __ibm128?

Insofar as that's what GCC on Power (for example, gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 from 2017) has shipped with for several years, yes.

In D94092#2481857, @aaron.ballman wrote:

In D94092#2481276, @rjmccall wrote:

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

Are you committed to the name __ibm128? I think a name that says something about floating point would be better than just a company name and a number. "double double" is the common name for this format pretty much everywhere, and while I certainly would *not* suggest actually spelling it double double (i.e. with repeated type specifiers0), you could certainly use something like __doubledouble.

I do feel like this is a terrible idea, sorry. It's a *very* niche use case to be dedicating a new compiler feature to, and it's a feature that demands a lot from the internal compiler architecture in ways that other features don't.

LGTM

People can't actually just copy/paste from the comment anyway because of the comment characters on the line, and I don't think anyone will misunderstand the example if the backslash is missing. It's silly that GCC has a problem with this, but since it does, let's just work around the problem and drop the backslash rather than having one comment block in the header use a different style.

Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression. At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema. At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.

Functionally LGTM. Minor suggestion.

LGTM

byval actually means it'll be passed on the stack, not as a pointer, so yes, that's a real change in conventions from an indirect argument.

Oh yes, somehow I overlooked that, sorry.

In D92361#2459513, @zoecarver wrote:

I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.

Sorry, I'm not sure I follow. Could you elaborate a bit or provide an example? What do you mean by "new" trivial_abi attribute?

I think that as long as the class leaves a copy/move constructor defaulted, there's no need for a new trivial_abi attribute.

Please test that there's actually an object declared and that it's not *just* a tag declaration.

I'm not entirely sure what you're still waiting for, really. It's a big patch with a lot of diffuse responsibilities, but you've gotten sign-off from individual people across at least most of it. Do you feel like there's something significant that hasn't been reviewed?

Seems fine to me.

Your patch description doesn't mention the changes to the inliner.

In D92361#2434376, @zoecarver wrote:

I think perhaps we are talking past each other and have reached the limits of what this sub-thread can hope to achieve.

I agree. I am sure that we (or at least you two) could talk about the semantics of "teleportation" and "destructively moving" objects for many many hours. But I'm not sure it would be productive to do so (at least in this review thread). So, Arthur, why don't you open a mailing list thread to continue discussing if you'd like.

And we can all agree, no matter what we decide to call it, that in the following snippet, S0 and S1 will be passed directly:

struct __attribute__((trivial_abi)) S0 {
  S0();
  S0(const S0 &) = delete;
  S0(S0 &&) = delete;
  int a;
};

struct S1 {
  S0 s0;
};

(Which lines up with how this currently works if instead of being deleted, the copy/move constructors of S0 were simply user-provided.)

In D92361#2433775, @Quuxplusone wrote:

In D92361#2433190, @rjmccall wrote:

There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some operation that creates the object at the new address and destroys it at the old.

That's where you're wrong (about C++). You might be right about C or Objective-C, I don't know. In C++, that "teleporting" happens without any call to a special member — there is no move happening, and no destroy happening. You can actually observe this: https://godbolt.org/z/zojooc The object is simply "bitwise-teleported" from one place to another. Standard C++17 says that this is a "guaranteed-copy-elision" context; there is indeed only one C++ "object" here. It just happens to blit around in memory beyond what the C++ code is doing to it.

There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some operation that creates the object at the new address and destroys it at the old. That operation is a destructive move (which I certainly did not originate as a term for this), or what your proposal calls a relocation (which may be a more palatable term for the C++ committee).

In D92361#2432578, @ahatanak wrote:

It seems like you are discussing the case where a class/struct annotated with trivial_abi contains a member that isn't destructively movable. In that case, clang correctly diagnoses it today. For example, if you remove the attribute from S2 in the above example and add it to S3 instead, it warns that trivial_abi cannot be applied to S3 because S2 is a non-trivial class type.

What I wasn't sure was whether S1 (which isn't annotated with trivial_abi in the original code I posted) should be treated as a destructively movable type despite having all its copy/move constructors deleted when its only member (s0) is destructively movable.

Based on the discussion we had a few years ago (http://lists.llvm.org/pipermail/cfe-dev/2017-November/055966.html), I think the answer is yes, but I just wanted to confirm.

In D92361#2427652, @zoecarver wrote:

If it is not possible to copy or move the type at all, it is not possible to copy or move it trivially.

This was a bit confusing to me because I think this changed between C++14 and 17. In C++14, a class was trivially copyable if it had no non-trivial copy/move constructors. But now the standard requires at least one non-trivial move/copy constructor. It looks like the type traits maybe haven't caught up.

That's an excellent example to study. The fundamental question we are looking to answer is whether a type representationally depends on its address. Absent the trivial_abi attribute, the criterion for knowing that it does not is whether it is possible to copy or move it trivially. If it is not possible to copy or move the type at all, it is not possible to copy or move it trivially. So S0 may representationally depend on its address, and we should ignore/diagnose trivial_abi on aggregates containing an S0.

This seems like a good change, but we should make sure we test cases that may previously have been covered by the implicit deletion of all copy/move constructors. That may just be a matter of auditing tests.

Functionally LGTM. I don't know if 11 is still taking changes, but if it is, you have code-owner approval.

I assume this has always been taken care of properly in the backend, so this is just a fix for va_arg treatment? If so, LGTM.

We don't usually add known-broken tests like this before a fix, as opposed to just landing them as part of the fix, but if you have a good reason to do so it's okay.

LGTM

LGTM

LGTM.

Probably should be pluralized for consistency, fast-honor-pragmas, but yeah, that's fine with me.

Oh, unfortunately llvm.coro.async.continuation wouldn't be able to just return the continuation function pointer; there would have to be a llvm.coro.async.get_continuation_function intrinsic. But we do something similar with the alloc intrinsics.

I see. This LGTM as an immediate fix, then.

What is this function being inlined? We expect the frontend to emit a thunk that glues things together?

fast-strict? Sounds sort of oxymoronic, but it's fast while also being strict about honoring pragmas. I don't have any great ideas here.

This is great work, thank you.

I don't quite understand the need to inline here.

Alright.

In D90612#2384323, @lxfind wrote:

Curious, is the plan to eventually replace .recon lowering with .async lowering?

LGTM

Okay. It sounds like strict compatibility with GCC implies ignoring pragmas in fast, and that's what we're most concerned with, since this is originally a GCC option. So the only question I have now is whether faststd is the best name for this. Does anyone want to suggest an alternative?

Looks good to me as a first pass.

Hmm. Do we actually want this behavior of fast overriding pragmas? What do other compilers do here? It might be reasonable to just treat this as a bug.

I agree this is useful. However, you need to update the manual to cover faststd.

Pointer authentication can also make structs non-trivial to copy.

I'm not sure how overlap-aware our packing is, but yeah, I'm fine with removing this.

I may not have been clear. I'm not saying SourceLocation is a meaningful concept in the driver. I'm saying that if you generalize the concept of "source location" to "location in the input", there is a clear analogue in the driver (namely, a position in the argument list), and the only reason this isn't passed down to the driver and used in diagnostics is that we don't have the ability to express that today to the diagnostic engine. So instead of trying to extract out a part of the diagnostics engine that will work without any concept of source locations, you should be trying to parameterize the diagnostics engine so that it can work with an arbitrary external concept of source locations, and then the driver can use a different kind of source location than the main compiler and everything is fine.

Alright, LGTM.

It sounds like what you want is a diagnostic library that's almost completely abstracted over the kinds of entities that can be stored in a diagnostic, including the definition of a source location. I don't think that's incompatible with this patch; there's no need to suggest reversion.

Patch generally looks good. Minor complaint about how you're using notes.

LGTM. Untyped pointers can't come soon enough.

Argh, sorry! I meant to say "I have *no* objections".

I have objections to the code change here. I'll leave the conceptual question to other people interested in the HIP toolchain.

Yes, there are no generically available libcalls for atomic float math -- but that's okay -- let LLVM handle transform into a cmpxchg loop when required.

In D71726#2207700, @yaxunl wrote:

clang does not always emit atomic instructions for atomic builtins. Clang may emit lib calls for atomic builtins. Basically clang checks target info about max atomic inline width and if the desired atomic operation exceeds the supported atomic inline width, clang will emit lib calls for atomic builtins. The rationale is that the lib calls may be faster than the IR generated by the LLVM pass. This behavior has long existed and it also applies to fp atomics. I don't think emitting lib calls for atomic builtins is a bug. However, this does introduce the issue about whether the library functions for atomics are available for a specific target. As I said, only the target owners have the answer and therefore I introduced the target hook.

LGTM.

Oh, I see, sorry.

Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by other compilers, so we need to coordinate. So you need to check out what other compilers do (GCC, ICC) and imitate them if they're already setting a reasonable default; otherwise, I think there's a standard algorithm for generating these.

Okay. Well, it does seem to me that we need to be considering lifetimes. If we can also optimize when we don't have lifetime information and the variable doesn't escape, that's great, but we often have pretty good lifetime information that we can use. And in optimized builds we'll usually have already eliminated allocas that don't escape, so the remaining allocas are very likely to have all escaped.

We do not actually support allocation failure for a lot of things around blocks. I don't think the copy-helper functions even have a way to propagate out a failure in copying a field. I have never seen any code in the wild that would handle Block_copy returning a null pointer. Effectively it is assumed to not happen.

There's an existing StackLifetime analysis that does some of this work and also considers the lifetime intrinsics; can we take advantage of that?

Of course, that wouldn't solve the general problem that the block might be getting used before its capture is fully initialized, but that's a general problem with uses within initializers in C.

It's not optimal, but an alternative would be to force the variable to the heap immediately rather than waiting for a potential block copy. The variable would actually be uninitialized during its "copy", so we'd need to give it a trivial copy helper. But once the variable's been moved to the heap, it's never copied again, so that's fine.

Okay. This LGTM if you feel that the operator forwarders is the better approach.

Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention.

We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard.

Okay, thanks. This LGTM.

No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored.

I agree that it seems reasonable to preserve an explicit CC from the lambda on the converted function pointer.

Even if we decide to change that recommendation, this is not the place to elaborate on the advice. We have an entire section of the developer's manual dedicated to providing guidance on choosing data structures, and we should not duplicate it clumsily and in part out here.

llvm::SmallVector isn't, like, otherwise worse than std::vector, so that you should only consider it over std::vector when the inline-capacity optimization is highly valuable. It does have some specific drawbacks, but we go into this in more detail in the programmer's manual. I think "prefer llvm::SmallVector" is fine general advice for this paragraph.

LGTM