Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (439 w, 3 d)

Recent Activity

Mon, Jun 14

rjmccall added a comment to D99696: [clang] NRVO: Improvements and handling of more cases..

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.

Mon, Jun 14, 8:49 AM · Restricted Project

Sat, Jun 12

rjmccall added inline comments to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.
Sat, Jun 12, 11:49 AM · Restricted Project

Sun, Jun 6

rjmccall accepted D101156: [Clang] Support a user-defined __dso_handle.

Great, LGTM.

Sun, Jun 6, 7:29 PM · Restricted Project

Fri, Jun 4

rjmccall accepted D103040: Print default template argument if manually specified in typedef declaration..

LGTM

Fri, Jun 4, 9:59 AM · Restricted Project
rjmccall added a comment to D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name..

Thanks, Akira.

Fri, Jun 4, 9:20 AM · Restricted Project
rjmccall added inline comments to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.
Fri, Jun 4, 9:20 AM · Restricted Project

Thu, Jun 3

rjmccall accepted D103603: [Sema][RISCV][SVE] Allow ?: to select Typedef BuiltinType in C.

LGTM

Thu, Jun 3, 6:46 PM · Restricted Project
rjmccall added inline comments to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.
Thu, Jun 3, 6:44 PM · Restricted Project
rjmccall added inline comments to D101156: [Clang] Support a user-defined __dso_handle.
Thu, Jun 3, 6:36 PM · Restricted Project
rjmccall added a comment to D103563: [HIP] Fix amdgcn builtin for long type.

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

Thu, Jun 3, 6:25 PM · Restricted Project

Wed, Jun 2

rjmccall added a reviewer for D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.: ahatanak.

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?

Wed, Jun 2, 10:33 AM · Restricted Project

Fri, May 28

rjmccall accepted D103285: [coro async] Add the swiftasync attribute to the resume partial function.

LGTM

Fri, May 28, 1:03 PM · Restricted Project
rjmccall added inline comments to D102689: [C++] Ignore top-level qualifiers in casts.
Fri, May 28, 10:03 AM · Restricted Project

Thu, May 27

rjmccall added a comment to D103275: Update musttail verification to check all swiftasync->swiftasync tail calls..

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.

Thu, May 27, 9:06 PM · Restricted Project

Wed, May 26

rjmccall added a comment to D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+..

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.

Wed, May 26, 7:33 PM · Restricted Project
rjmccall accepted D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name.

LGTM, thanks!

Wed, May 26, 7:24 PM · Restricted Project
rjmccall added a comment to D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name.

Thanks, that seems to work out cleanly.

Wed, May 26, 3:44 PM · Restricted Project
rjmccall added inline comments to D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name.
Wed, May 26, 11:40 AM · Restricted Project

Tue, May 25

rjmccall added a comment to D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name..

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.

Tue, May 25, 11:58 PM · Restricted Project
rjmccall added a comment to D102478: [Matrix] Emit assumption that matrix indices are valid..

Do you want to generate these even at -O0?

Tue, May 25, 11:51 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name.
Tue, May 25, 11:46 PM · Restricted Project
rjmccall accepted D102996: [ObjC][ARC] Use the addresses of the ARC runtime functions instead of integer 0/1 for the operand of bundle "clang.arc.attachedcall".

LGTM

Tue, May 25, 11:20 PM · Restricted Project
rjmccall added a comment to D102443: [PowerPC] Added multiple PowerPC builtins.

Alright, that's fine with me, too.

Tue, May 25, 11:19 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D90868: [IR] Define @llvm.ptrauth intrinsics..
Tue, May 25, 11:16 PM · Restricted Project
rjmccall added a comment to D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+..

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.

Tue, May 25, 11:08 PM · Restricted Project

Mon, May 24

rjmccall added a comment to D102689: [C++] Ignore top-level qualifiers in casts.

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.
  1. 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.
Mon, May 24, 3:14 PM · Restricted Project
rjmccall added a reviewer for D102689: [C++] Ignore top-level qualifiers in casts: rsmith.

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

Mon, May 24, 1:17 PM · Restricted Project
rjmccall added a comment to D102443: [PowerPC] Added multiple PowerPC builtins.

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

Mon, May 24, 12:43 PM · Restricted Project, Restricted Project

May 20 2021

rjmccall added a comment to D102443: [PowerPC] Added multiple PowerPC builtins.

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.

May 20 2021, 1:39 AM · Restricted Project, Restricted Project

May 14 2021

rjmccall accepted D102015: [clang CodeGen] Don't crash on large atomic function parameter..

Thanks, LGTM

May 14 2021, 2:59 PM · Restricted Project

May 12 2021

rjmccall added a comment to D101980: [RFC] [Coroutines] Put the contents of byval argument to the frame.

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.

May 12 2021, 12:42 AM · Restricted Project

May 11 2021

rjmccall added a comment to D101980: [RFC] [Coroutines] Put the contents of byval argument to the frame.

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.

May 11 2021, 7:32 PM · Restricted Project
rjmccall accepted D101968: Fix bad mangling of <data-member-prefix> for a closure in the initializer of a variable at global namespace scope..

LGTM

May 11 2021, 7:03 PM · Restricted Project
rjmccall added a comment to D101980: [RFC] [Coroutines] Put the contents of byval argument to the frame.

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.

May 11 2021, 10:17 AM · Restricted Project

May 10 2021

rjmccall added a comment to D101980: [RFC] [Coroutines] Put the contents of byval argument to the frame.

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.

May 10 2021, 11:24 PM · Restricted Project
rjmccall added a comment to D102015: [clang CodeGen] Don't crash on large atomic function parameter..

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.

May 10 2021, 10:50 PM · Restricted Project

May 6 2021

rjmccall added a comment to D101980: [RFC] [Coroutines] Put the contents of byval argument to the frame.

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.

May 6 2021, 9:37 PM · Restricted Project
rjmccall added a comment to D102015: [clang CodeGen] Don't crash on large atomic function parameter..

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.

May 6 2021, 9:19 PM · Restricted Project
rjmccall accepted D101501: [CGAtomic] Lyft strong requirement for remaining compare_exchange combinations.

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

May 6 2021, 8:58 PM · Restricted Project
rjmccall added inline comments to D101841: [Coroutines] Do not add alloca to the frame if the size is 0.
May 6 2021, 12:38 PM · Restricted Project

May 4 2021

rjmccall added a comment to D101501: [CGAtomic] Lyft strong requirement for remaining compare_exchange combinations.

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.

May 4 2021, 7:38 PM · Restricted Project
rjmccall accepted D101841: [Coroutines] Do not add alloca to the frame if the size is 0.

Great, thanks.

May 4 2021, 11:02 AM · Restricted Project
rjmccall added a comment to D101841: [Coroutines] Do not add alloca to the frame if the size is 0.

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.

May 4 2021, 10:01 AM · Restricted Project

May 3 2021

rjmccall added a comment to D101501: [CGAtomic] Lyft strong requirement for remaining compare_exchange combinations.

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.

May 3 2021, 7:38 PM · Restricted Project
rjmccall added a comment to D101501: [CGAtomic] Lyft strong requirement for remaining compare_exchange combinations.

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

May 3 2021, 5:42 PM · Restricted Project
rjmccall added inline comments to D76526: Add an algorithm for performing "optimal" layout of a struct..
May 3 2021, 5:26 PM · Restricted Project

Apr 30 2021

rjmccall accepted D36368: Fix type printing of array template args.

Thanks, LGTM

Apr 30 2021, 11:55 AM · Restricted Project, Restricted Project

Apr 29 2021

rjmccall added inline comments to D36368: Fix type printing of array template args.
Apr 29 2021, 11:53 PM · Restricted Project, Restricted Project
rjmccall added a comment to D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+..

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

Apr 29 2021, 1:55 PM · Restricted Project
rjmccall added inline comments to D36368: Fix type printing of array template args.
Apr 29 2021, 11:42 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D36368: Fix type printing of array template args.
Apr 29 2021, 1:17 AM · Restricted Project, Restricted Project

Apr 28 2021

rjmccall added a comment to D101156: [Clang] Support a user-defined __dso_handle.

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.

Apr 28 2021, 10:50 PM · Restricted Project
rjmccall accepted D101097: [Sema] Don't set BlockDecl's DoesNotEscape bit If the block is being passed to a function taking a reference parameter.

LGTM

Apr 28 2021, 10:18 PM · Restricted Project
rjmccall added a comment to D101389: [clang][CodeGen] Fix address space for sret.

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?

Apr 28 2021, 10:17 PM
rjmccall added a comment to D101389: [clang][CodeGen] Fix address space for sret.

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++.

Apr 28 2021, 7:05 PM
rjmccall accepted D101502: [ObjC][ARC] Don't enter the cleanup scope if the initializer expression isn't an ExprWithCleanups .

Okay, LGTM.

Apr 28 2021, 7:03 PM · Restricted Project
rjmccall added a comment to D100322: [ConstantMerge] Don't merge thread_local constants with non-thread_local constants.

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.

Apr 28 2021, 1:19 PM · Restricted Project

Apr 27 2021

rjmccall added inline comments to D36368: Fix type printing of array template args.
Apr 27 2021, 10:04 AM · Restricted Project, Restricted Project

Apr 26 2021

rjmccall added a comment to D100739: [Coroutines] Handle overaligned frame allocation (2).

Here are the options I think the committee might take:

Apr 26 2021, 9:50 PM · Restricted Project, Restricted Project
rjmccall added a comment to D100739: [Coroutines] Handle overaligned frame allocation (2).

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.

Apr 26 2021, 12:24 PM · Restricted Project, Restricted Project
rjmccall added a comment to D100739: [Coroutines] Handle overaligned frame allocation (2).

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

Apr 26 2021, 10:12 AM · Restricted Project, Restricted Project

Apr 23 2021

rjmccall added inline comments to D101097: [Sema] Don't set BlockDecl's DoesNotEscape bit If the block is being passed to a function taking a reference parameter.
Apr 23 2021, 2:58 PM · Restricted Project
rjmccall accepted D100879: [Clang] Propagate guaranteed alignment for malloc and others .

Thanks, LGTM.

Apr 23 2021, 12:56 AM · Restricted Project

Apr 22 2021

rjmccall added a comment to D100739: [Coroutines] Handle overaligned frame allocation (2).

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.

Apr 22 2021, 11:49 PM · Restricted Project, Restricted Project
rjmccall updated subscribers of D93377: [Clang] Add __ibm128 type to represent ppc_fp128.
Apr 22 2021, 11:32 PM · Restricted Project
rjmccall added inline comments to D101097: [Sema] Don't set BlockDecl's DoesNotEscape bit If the block is being passed to a function taking a reference parameter.
Apr 22 2021, 9:59 PM · Restricted Project
rjmccall added a comment to D100879: [Clang] Propagate guaranteed alignment for malloc and others .

Test looks good, thanks.

Apr 22 2021, 9:40 PM · Restricted Project

Apr 20 2021

rjmccall added a comment to D100879: [Clang] Propagate guaranteed alignment for malloc and others .

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

Apr 20 2021, 9:15 PM · Restricted Project

Apr 19 2021

rjmccall added a comment to D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.

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

Apr 19 2021, 11:18 AM · Restricted Project, Restricted Project

Apr 16 2021

rjmccall added a comment to D96033: [clang-repl] Land initial infrastructure for incremental parsing.

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

Apr 16 2021, 12:57 PM · Restricted Project
rjmccall added a comment to D96033: [clang-repl] Land initial infrastructure for incremental parsing.

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.

Apr 16 2021, 1:07 AM · Restricted Project
rjmccall added a comment to D99790: [CGCall] Annotate `this` argument with alignment.

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.

Apr 16 2021, 12:00 AM · Restricted Project, Restricted Project

Apr 15 2021

rjmccall added a comment to D100591: [Clang][AArch64] Disable rounding of return values for AArch64.

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.

Apr 15 2021, 7:06 PM · Restricted Project
rjmccall added a comment to D100591: [Clang][AArch64] Disable rounding of return values for AArch64.

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

Apr 15 2021, 12:17 PM · Restricted Project
rjmccall added a comment to D76526: Add an algorithm for performing "optimal" layout of a struct..

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?

Apr 15 2021, 11:04 AM · Restricted Project

Apr 13 2021

rjmccall added a comment to D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.

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.

Apr 13 2021, 8:15 PM · Restricted Project, Restricted Project
rjmccall accepted D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.

LGTM

Apr 13 2021, 2:38 PM · Restricted Project, Restricted Project

Apr 11 2021

rjmccall added a comment to D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.

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.)

Apr 11 2021, 10:58 PM · Restricted Project, Restricted Project
rjmccall added a comment to D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.

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.

Apr 11 2021, 10:41 PM · Restricted Project, Restricted Project
rjmccall added a comment to D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass.

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.

Apr 11 2021, 10:20 PM · Restricted Project, Restricted Project
rjmccall added a reviewer for D100225: [Clang][AArch64] Coerce integer return values through an undef vector: t.p.northover.

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

Apr 11 2021, 1:51 PM · Restricted Project
rjmccall added a comment to D100225: [Clang][AArch64] Coerce integer return values through an undef vector.

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

Apr 11 2021, 11:56 AM · Restricted Project
rjmccall added a comment to D100225: [Clang][AArch64] Coerce integer return values through an undef vector.

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

Apr 11 2021, 11:09 AM · Restricted Project

Apr 10 2021

rjmccall added inline comments to D76526: Add an algorithm for performing "optimal" layout of a struct..
Apr 10 2021, 11:13 PM · Restricted Project
rjmccall added a comment to D36368: Fix type printing of array template args.

Sorry, this got lost.

Apr 10 2021, 11:09 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D76526: Add an algorithm for performing "optimal" layout of a struct..
Apr 10 2021, 9:15 PM · Restricted Project
rjmccall added a comment to D60456: [RISCV] Hard float ABI support.

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

Apr 10 2021, 2:09 AM · Restricted Project, Restricted Project

Apr 8 2021

rjmccall accepted D100134: [libc++] Fix std::type_info comparison.

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

Apr 8 2021, 3:54 PM · Restricted Project

Apr 7 2021

rjmccall added a comment to D97802: [libc++] Increase readability of typeinfo comparison of ARM64.

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.

Apr 7 2021, 1:45 PM · Restricted Project
rjmccall added a comment to D100037: [clang][UBSan] Passing underaligned pointer as a function argument is undefined behaviour.

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".

Apr 7 2021, 12:08 PM · Restricted Project, Restricted Project, Restricted Project

Apr 6 2021

rjmccall added a comment to D99179: [RFC] [Coroutines] Enable printing coroutine frame in debugger if program is compiled with -g.

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.

Apr 6 2021, 7:32 PM · debug-info, Restricted Project
rjmccall added a comment to D99791: [CGCall] Annotate pointer argument with alignment.

@rjmccall thank you for taking a look!

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.

Apr 6 2021, 4:14 PM · Restricted Project, Restricted Project
rjmccall accepted D99898: [clang, test] Fix use of undef FileCheck var.

Thanks!

Apr 6 2021, 2:16 PM · Restricted Project
rjmccall added a comment to D99893: [WIP] Replace std::forward & std::move by cast expressions during Sema.

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.

Apr 6 2021, 12:21 PM · Restricted Project
rjmccall added a comment to D99898: [clang, test] Fix use of undef FileCheck var.

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

Apr 6 2021, 12:15 PM · Restricted Project
rjmccall accepted D71726: Let clang atomic builtins fetch add/sub support floating point types.

Thanks, LGTM

Apr 6 2021, 11:52 AM · Restricted Project

Apr 5 2021

rjmccall added a comment to D99791: [CGCall] Annotate pointer argument with alignment.

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

Apr 5 2021, 11:52 PM · Restricted Project, Restricted Project
rjmccall added a comment to D71726: Let clang atomic builtins fetch add/sub support floating point types.

Alright, mostly looks good.

Apr 5 2021, 11:37 PM · Restricted Project
rjmccall added a comment to D78564: [i386] Modify the alignment of __m128/__m256/__m512 vector type according i386 abi..

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

Apr 5 2021, 10:43 PM · Restricted Project
rjmccall added a comment to D99893: [WIP] Replace std::forward & std::move by cast expressions during Sema.

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.

Apr 5 2021, 12:46 PM · Restricted Project