When __block variables get moved to the heap, they're supposed to be moved if possible, not copied. It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move. Maybe it's being passed in wrong when building __block copy expressions in some situation.

Great, LGTM.

LGTM

Thanks, Akira.

LGTM

If you want a way to say "some 64-bit type" in the builtins file, we could add that.

Hmm. CC'ing Akira. Akira, do you know why we're building a fake FunctionDecl as part of emitting these helper functions? Is it something we can avoid?

LGTM

I would be uncomfortable with an invariant that all calls from swifttailcc to swifttailcc must be musttail because we might well want to make non-tail calls in some circumstances. However, I think it would be fine to annotate all such calls as either musttail or a hypothetical notail — at least, I think we could achieve that coming out of the frontend, and we ought to be able to maintain it in IR transformations. To do that, you'd need to actually add a notail, though.

There's no reliable way to report this with UBSan because in general we can't ever know that a loop will not terminate. That said, we could report *obviously* trivial loops, either with UBSan or just with a diagnostic. If the body of your loop really is empty, and the conditions are trivial, that ought to be enough to catch it.

LGTM, thanks!

Thanks, that seems to work out cleanly.

In D98799#2761684, @dblaikie wrote:

OK - poked around a bit more to better understand this, so attempting to summarize my current understanding (excuse the repetition from previous parts of this thread) and results.

• __attribute__((overloadable)) can/does mangle K&R C style declarations
• with this patch (currently committed), -funique-internal-linkage-names does not mangle K&R C style declarations (see bar in the included test case, unmangled)
• I'd like to avoid that divergence if possible
• Changing the debug info code to be more generous with names it mangles (by using FD->getType()->getAs<FunctionProtoType>() rather than hasPrototype()) causes problems
  • Specifically: Objective C blocks (which have a FunctionProtoType type, but !hasPrototype it seems) are missing parameter info so this call crashes
    • There doesn't seem to be any way to test for this property of the FunctionDecl that I can see - where it has a type, but doesn't have parameter info

Trying to pull in some folks who might know what's going on here/be able to suggest a way to split these cases if needed, or fix the block FunctionDecls to have param info. @rjmccall @JDevlieghere - I'd really appreciate some help here.

Do you want to generate these even at -O0?

LGTM

Alright, that's fine with me, too.

In D96418#2780687, @leonardchan wrote:

Hi all, it looks like this commit led to some unexpected behavior in our build. When compiling something like:

// clang++ -c -S -o - /tmp/test.cc -std=c++17 -O1
static int do_nothing(void* unused) {
  for (;;) {
  }
  return 0;
}

typedef int (*func_t)(void *);
void call(func_t);

void func() {
  call(&do_nothing);
}

we get the following assembly for do_nothing:

	.p2align	4, 0x90                         # -- Begin function _ZL10do_nothingPv
	.type	_ZL10do_nothingPv,@function
_ZL10do_nothingPv:                      # @_ZL10do_nothingPv
	.cfi_startproc
# %bb.0:
.Lfunc_end1:
	.size	_ZL10do_nothingPv, .Lfunc_end1-_ZL10do_nothingPv
	.cfi_endproc
                                        # -- End function
	.ident	"Fuchsia clang version 13.0.0 (https://llvm.googlesource.com/a/llvm-project 6555e53ab0f2bca3dc30f5ab3a2d6872d50b6ff8)"
	.section	".note.GNU-stack","",@progbits
	.addrsig
	.addrsig_sym _ZL10do_nothingPv

It seems that the function doesn't have a return statement or halting instruction and it would just jump into the next function. While I do follow what this patch is trying to do, this behavior seems pretty unexpected from a C++ user perspective. I could be wrong, but it doesn't seem clear in this case that the infinite loop results in UB which would justify this assembly.

In D102689#2778072, @rsmith wrote:

In D102689#2778011, @rjmccall wrote:

The C++ standard does not appear to have similar wording. On the other hand, the C++ standard says that e.g. "The result of the expression (T) cast-expression is of type T", and similarly for the other casts, which is clearly just wrong if T is a reference type; the wording clarifies that the expression is an l-value or x-value if the type is a reference but doesn't remove the reference-ness of the expression type as it must, unless that's done by some other clause at a distance.

It is indeed done by a different clause at a distance:

[expr.type]

1. If an expression initially has the type “reference to T”, the type is adjusted to T prior to any further analysis. The expression designates the object or function denoted by the reference, and the expression is an lvalue or an xvalue, depending on the expression.

2. If a prvalue initially has the type “cv T”, where T is a cv-unqualified non-class, non-array type, the type of the expression is adjusted to T prior to any further analysis.

Top-level qualifiers aren't normally meaningful on pr-values.

The Clang part of this looks fine to me; I can't review the backend changes, but you have partial approval.

If these builtins are generally supposed to be accessed through a compiler-provided header, you could make that header define these functions using underlying builtins that have a __builtin prefix. That's not completely necessary, though; we do support some other non-library builtins that lack the __builtin prefix. And it sounds like maybe there isn't such a header anyway.

Thanks, LGTM

In D101980#2752963, @ChuanqiXu wrote:

In D101980#2752775, @rjmccall wrote:

That are semantically passed by value but "physically" passed as a pointer to a temporary? Yes, absolutely, depending on the target. Example: https://godbolt.org/z/oK1Yadjvq

(I assume you meant "fine".) That's true, no. It is *uncommon* for a type to have a copy/move operation that isn't semantically equivalent to memcpy without also having a non-trivial destructor, but it's absolutely possible, both formally and in practice. Practical examples include relative pointers or structs containing a field signed with address-diversified pointer authentication.

In D101980#2752763, @ChuanqiXu wrote:

In D101980#2751187, @rjmccall wrote:

Implicitly by reference, i.e. the argument type is not a reference type but the ABI just passes a pointer behind the scenes.

Does it mean there are arguments who should be passed by value but are passed by reference actually? If it is true, we are in trouble.

LGTM

In D101980#2750311, @ChuanqiXu wrote:

Thanks for the detailed explanation! I still had some questions.

An indirect argument is one that's implicitly passed by reference

Do you mean by value here? If an argument is passed by reference, it makes no sense the coroutine need to copy the argument by value. Since the reference is pointer really. And I think it is responsibility for the programmers to maintain the lifetime for the pointer. And the programmers should agree on there is only pointer in the coroutine frame.

In D101980#2743798, @ChuanqiXu wrote:

In D101980#2743665, @rjmccall wrote:

How is the situation with byval different from the situation with indirect arguments? It sounds like we generally need to perform this extra copy into the coroutine frame once it's allocated, and yes, that's something we need to emit in the frontend because it might be non-trivial.

Or is the problem that LLVM might be helpfully eliminating the copy when the source is a byval parameter? But that's also something it could be doing with indirect arguments, so again, treating byval specially seems suspect.

Sorry. May I ask what does indirect argument mean? I tried to copy all byval parameters since it looks like if the frontend needs to pass parameters by value for structs, it would emit byval attributes.

In D102015#2748441, @efriedma wrote:

In D102015#2743634, @rjmccall wrote:

Objective-C object types also have aggregate evaluation kind. Those can only be used as value types in an old, fragile ObjC ABI, but I believe we haven't yet formally decided to remove support for that. Fortunately, however, there's a much better simplification available: this hasAggregateEvaluationKind(Ty) call is literally doing nothing that couldn't just be a getAs<RecordType>() call, and then we don't need to worry about it.

If you're confident that only RecordTypes need to be destroyed, sure.

How is the situation with byval different from the situation with indirect arguments? It sounds like we generally need to perform this extra copy into the coroutine frame once it's allocated, and yes, that's something we need to emit in the frontend because it might be non-trivial.

Objective-C object types also have aggregate evaluation kind. Those can only be used as value types in an old, fragile ObjC ABI, but I believe we haven't yet formally decided to remove support for that. Fortunately, however, there's a much better simplification available: this hasAggregateEvaluationKind(Ty) call is literally doing nothing that couldn't just be a getAs<RecordType>() call, and then we don't need to worry about it.

LGTM, except that the commit message should say "Lift", not "Lyft". ;)

In D101501#2737180, @jfb wrote:

In D101501#2735352, @rjmccall wrote:

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Ugh right you are ☹️
I have a small soapbox about making this stuff a template parameter but I'll keep it to myself.

Great, thanks.

Can you test this with some code that doesn't bitcast the zero-sized alloca before its use? Just slightly worried that we might assert because of the type difference.

Don't mind JF, he's just forgotten that C++ lacks the ability to turn the typically constant value arguments passed to functions like std::atomic::compare_exchange_strong into actual compile-time constants to the builtin used within those functions, so his advice would mean emitting all atomics in C++ as sequentially consistent.

Your change is correct, but I think there's a deeper flaw in this code.

Thanks, LGTM

Seems conceptually okay, given that we have an option to control it.

What's the crash exactly/ Is IRGen just unhappy about processing the user definition when we've generated a declaration with a different type? Because we're already supposed to be being cautious about this.

LGTM

In D101389#2724700, @yaxunl wrote:

In D101389#2724636, @rjmccall wrote:

I think this is intentional; requiring the indirect-result parameter to be in the alloca address space would prevent direct initialization of non-temporary memory, which is an important optimization in C++.

You mean situations like this? https://godbolt.org/z/KnPs6znK8

Address of a global variable is directly passed as the sret arg to the function returning a struct, instead of creating a temporary struct variable and passing its address as the sret arg?

I think this is intentional; requiring the indirect-result parameter to be in the alloca address space would prevent direct initialization of non-temporary memory, which is an important optimization in C++.

Okay, LGTM.

In D100322#2723566, @Amanieu wrote:

I'm not even sure what it would *mean* to have an unnamed TLS constant: I would expect an earlier pass to convert it to a normal constant rather than having to it in ConstantMerge.

Also, AFAIK neither clang nor rustc will ever generate unnamed TLS constants.

Here are the options I think the committee might take:

In D100739#2717259, @ychen wrote:

In D100739#2717227, @rjmccall wrote:

Yes, if you can dynamically choose to use an aligned allocator, that's clearly just much better.

Right now:

Intrinsic::coro_size_aligned : overaligned frame: over-allocate, adjust start address; non-overaligned frame: no-op
Intrinsic::coro_size : overaligned frame: no-op; non-overaligned frame: no-op

Do you mean to remove Intrinsic::coro_size_aligned and make
Intrinsic::coro_size : overaligned frame: over-allocate, adjust start address; non-overaligned frame: no-op

that makes sense to me. Just want to confirm first.

Yes, if you can dynamically choose to use an aligned allocator, that's clearly just much better.

Thanks, LGTM.

What is the purpose of the builtin? Where is it being used? Typically you *can't* change the signature of a builtin because the builtin is itself a language feature that's documented to have a particular signature. If you've made a builtin purely for use in generated AST, that's pretty unfortunate, and you should consider whether you actually have to do that instead of e.g. synthesizing a call to an allocation function the same way that we do in new expressions.

Test looks good, thanks.

Please addd tests, including tests that we suppress this assumption under e.g. -fno-builtin-malloc

MLIR is an in-tree project that can be updated.

In D96033#2695622, @v.g.vassilev wrote:

Would it make sense to have each Decl point to its owning PTU, similarly to what we do for the owning module (Decl::getOwningModule)?

Sorry for the delay. That seems like a reasonable summary of our discussion. Let me try to lay out the right architecture for this as I see it.

I agree. The arrangement logic is assuming that the exact pointer type doesn't matter because it's all just a pointer in the end, but of course that breaks down when we start inferring attributes.

In D100591#2692978, @asavonic wrote:

In D100591#2692599, @rjmccall wrote:

I think the right thing to do here is to recognize generally that we're emitting a mandatory tail call, and so suppress *all* the normal transformations on the return value.

I assume it can be tricky to detect such call. The final decision (tail call vs normal call) is made before instruction selection, after all LLVM IR optimization passes. So we can miss tail calls that are not obvious on non-optimized code, or get false-positive results for calls that a backend decides to emit as normal calls.

Hmm. I think the right thing to do here is to recognize generally that we're emitting a mandatory tail call, and so suppress *all* the normal transformations on the return value. The conditions on mandatory tail calls should make that possible, and it seems like it would be necessary for a lot of types. Aggregates especially come to mind — if an aggregate is returned in registers, we're probably going to generate code like

In D76526#2689767, @dblaikie wrote:

In D76526#2689041, @lxfind wrote:

In D76526#2688002, @dblaikie wrote:

Out of curiosity, what's the motivating use case for the fixed offset fields in this tool?

There is a need for this in coroutines. In the coroutine frame layout, the first few fields are fixed according to C++ standard.

Ah, makes sense - is that only for prefixes, or can that result in arbitrary non-initial-sequence fixed offset fields too?

That something is an (unlowered) coroutine is an important semantic property of the function that should be represented more explicitly in IR than just whether it contains a call to an intrinsic in the llvm.coro.id family. Coroutines have somewhat different structural rules from ordinary functions, as is natural for a somewhat different high-level concept. noinline is an independent attribute that should affect whether the ramp function is inlinable after coroutine lowering is complete.

LGTM

We don't have so many coroutine-emitting frontends that it's unreasonable to modify them all. Swift can make this change; it's not on you to do it. (I also work on the Swift frontend, so don't worry, I'm signing my own teammates up for work here.)

In D100282#2682269, @lxfind wrote:

In D100282#2682251, @rjmccall wrote:

Why does this pass even exist? We should just expect the frontend to set the attribute. It's not like frontends don't have to otherwise know that they're emitting a coroutine; a ton of things about the expected entire IR pattern are different.

The attribute setting can totally be moved to the front-end.
One thing that's not clear to me is whether we should simply set coroutine functions noinline instead of replying on the attributres for this.
GCC seems to complain about inlining coroutines: https://godbolt.org/z/KrzE1znno, not fully sure why.

Why does this pass even exist? We should just expect the frontend to set the attribute. It's not like frontends don't have to otherwise know that they're emitting a coroutine; a ton of things about the expected entire IR pattern are different.

You should probably talk it over with AArch64 backend folks, but yes, abstractly it seems reasonable. CC'ing Tim Northover.

AArch64 only has 64-bit integer registers, so of course the algorithm is specified that way. LLVM could nonetheless choose to return an i32.

Why does the ABI "require" this to be returned as an i64 if some of the bits are undefined?

Sorry, this got lost.

Your use of CoerceAndExpand seems fine; thanks for pinging me on it

Patch functionally LGTM. Not sure what you're asking about the test script, but I don't know the libc++ test suite.

Wow, yes, you're absolutely right. I think we can probably get away with fixing that without worrying about ODR problems because type_info ordered comparison is so uncommon.

Yes. We now have a motivational reason to do so, it wouldn't be "just expose the UB for the sake of miscompiling the program".

Normally we don't want passes making educated guesses about things and codifying that into debug info, but in this specific instance i don't think it's unreasonable, since the debug frame is an artificial type. Ideally you would find the debug info that's present and use that to describe the frame layout as much as possible. The way frame layout works should allow that kind of reversal well enough.

In D99791#2672528, @lebedev.ri wrote:

@rjmccall thank you for taking a look!

In D99791#2670333, @rjmccall wrote:

The last major conversation we had about this was this RFC I sent out about five years ago:

https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html

In that RFC, I specifically argued that we should not do this, and that it should only be considered undefined behavior to actually access memory through a misaligned pointer (for a particular definition of "access memory"). At Apple, we have made that a promise to our internal users, so even if we decide to do this, we will need to not do it on Darwin. However, as I remember it, the LLVM community did not reach a consensus to adopt my recommendation, so in principle we still have the flexibility to start doing this. I continue to believe that doing so would be a mistake. At any rate, you should start by reading that thread.

Thank you for the pointer!
I can for sure provide an opt-out, however note that in the end it will cause performance regressions as compared to the current LLVM optimizations.

Thanks!

Our builtin logic is capable of recognizing user declarations as builtins without any sort of explicit annotation in source. That's how we recognize C library builtins, for example. As Richard points out, that logic isn't really set up to do the right thing for namespaced declarations (or template ones, for that matter), but that seems fixable.

NUW_RN isn't a good name for the variable in this alternative since it's not readnone anymore. With an appropriate rename, LGTM.

Thanks, LGTM

The last major conversation we had about this was this RFC I sent out about five years ago:

Alright, mostly looks good.

Patch LGTM. If this is what GCC is doing on Linux, then we should match it.

I think a pattern-match in IRGen is probably a much more reasonable alternative if the goal is primarily to avoid the code-generation overheads at -O0 / optimization costs at -O<n>. You'd still have template instantiation overheads; I don't know how significant those really are.