Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (417 w, 2 d)

Recent Activity

Sat, Jan 16

rjmccall added a comment to D94854: [Clang] Fix SwiftCallingConv's aggregate lowering for _Atomic(_Bool)..

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.

Sat, Jan 16, 10:57 PM · Restricted Project

Fri, Jan 15

rjmccall added a comment to D94854: [Clang] Fix SwiftCallingConv's aggregate lowering for _Atomic(_Bool)..

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.

Fri, Jan 15, 9:27 PM · Restricted Project

Wed, Jan 13

rjmccall added a comment to D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1.

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.

Wed, Jan 13, 10:59 AM · Restricted Project, Restricted Project

Thu, Jan 7

rjmccall accepted D94257: [test] Move coro-retcon-unreachable.ll into llvm/test.

Yeah, that's definitely an LLVM test.

Thu, Jan 7, 1:46 PM · Restricted Project, Restricted Project
rjmccall accepted D92409: [AST][NFC] Silence GCC warning about multiline comments.

Okay, LGTM.

Thu, Jan 7, 8:56 AM · Restricted Project
rjmccall added a comment to D89490: Introduce __attribute__((darwin_abi)).

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.

Thu, Jan 7, 1:56 AM · Restricted Project, Restricted Project

Wed, Jan 6

rjmccall accepted D94196: [NFC] Move readAPValue/writeAPValue up the inheritance hierarchy.

LGTM

Wed, Jan 6, 3:26 PM · Restricted Project
rjmccall added a comment to D89490: Introduce __attribute__((darwin_abi)).

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.

Wed, Jan 6, 10:24 AM · Restricted Project, Restricted Project
rjmccall added a comment to D93377: [Clang] Add __ibm128 type to represent ppc_fp128.

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.

Wed, Jan 6, 10:17 AM · Restricted Project
rjmccall added a comment to D94092: [Clang] Remove unnecessary Attr.isArgIdent checks..

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.

Wed, Jan 6, 10:13 AM · Restricted Project

Tue, Jan 5

rjmccall added a comment to D93377: [Clang] Add __ibm128 type to represent ppc_fp128.

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.

Tue, Jan 5, 8:48 PM · Restricted Project
rjmccall added a comment to D89490: Introduce __attribute__((darwin_abi)).

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.

Tue, Jan 5, 8:39 PM · Restricted Project, Restricted Project
rjmccall accepted D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array..

LGTM

Tue, Jan 5, 8:30 PM · Restricted Project
rjmccall added a comment to D92409: [AST][NFC] Silence GCC warning about multiline comments.

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.

Tue, Jan 5, 8:28 PM · Restricted Project
rjmccall added a comment to D94092: [Clang] Remove unnecessary Attr.isArgIdent checks..

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.

Tue, Jan 5, 8:24 PM · Restricted Project

Wed, Dec 23

rjmccall accepted D93273: [CodeGen][ObjC] Destroy callee-destroyed arguments in the caller function when the receiver is nil.

Functionally LGTM. Minor suggestion.

Wed, Dec 23, 2:51 PM · Restricted Project

Tue, Dec 22

rjmccall accepted D93568: Add a llvm.coro.end.async intrinsic.
Tue, Dec 22, 10:45 AM · Restricted Project

Dec 17 2020

rjmccall accepted D92577: Don't reject tag declarations in for loop clause-1.

LGTM

Dec 17 2020, 2:36 PM
rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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.

Dec 17 2020, 10:40 AM · Restricted Project
rjmccall added inline comments to D92577: Don't reject tag declarations in for loop clause-1.
Dec 17 2020, 10:38 AM
rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

Oh yes, somehow I overlooked that, sorry.

Dec 17 2020, 10:02 AM · Restricted Project

Dec 16 2020

rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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?

Dec 16 2020, 11:37 PM · Restricted Project
rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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

Dec 16 2020, 3:02 PM · Restricted Project
rjmccall added a comment to D92577: Don't reject tag declarations in for loop clause-1.

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

Dec 16 2020, 2:37 PM
rjmccall added a comment to D54749: Saturating float to int casts..

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?

Dec 16 2020, 10:13 AM · Restricted Project

Dec 10 2020

rjmccall accepted D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types..

Seems fine to me.

Dec 10 2020, 6:07 PM · Restricted Project

Dec 9 2020

rjmccall added inline comments to D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types..
Dec 9 2020, 7:28 PM · Restricted Project

Dec 8 2020

rjmccall added a comment to D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR.

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

Dec 8 2020, 2:28 PM · Restricted Project, Restricted Project

Dec 4 2020

rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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

Dec 4 2020, 12:03 PM · Restricted Project
rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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.

Dec 4 2020, 10:49 AM · Restricted Project
rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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

Dec 4 2020, 12:11 AM · Restricted Project

Dec 3 2020

rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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.

Dec 3 2020, 5:15 PM · Restricted Project

Dec 2 2020

rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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.

Dec 2 2020, 1:31 AM · Restricted Project

Dec 1 2020

rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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.

Dec 1 2020, 6:34 PM · Restricted Project
rjmccall added inline comments to D92361: [trivial-abi] Support types without a copy or move constructor..
Dec 1 2020, 9:51 AM · Restricted Project

Nov 30 2020

rjmccall added inline comments to D92361: [trivial-abi] Support types without a copy or move constructor..
Nov 30 2020, 11:46 PM · Restricted Project
rjmccall added inline comments to D92361: [trivial-abi] Support types without a copy or move constructor..
Nov 30 2020, 9:02 PM · Restricted Project
rjmccall added a comment to D92361: [trivial-abi] Support types without a copy or move constructor..

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.

Nov 30 2020, 8:22 PM · Restricted Project
rjmccall added a comment to D91874: [GNU ObjC] Fix a regression listing methods twice..

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

Nov 30 2020, 12:24 PM · Restricted Project

Nov 18 2020

rjmccall accepted D91596: [PowerPC] [Clang] Fix alignment of 128-bit floating types.

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.

Nov 18 2020, 7:37 PM · Restricted Project
rjmccall added inline comments to D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments..
Nov 18 2020, 11:19 AM · Restricted Project, Restricted Project

Nov 17 2020

rjmccall accepted D91269: [Clang][CodeGen][RISCV] Add hard float ABI tests with empty struct.

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.

Nov 17 2020, 8:34 PM · Restricted Project
rjmccall added inline comments to D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments..
Nov 17 2020, 6:11 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments..
Nov 17 2020, 3:44 PM · Restricted Project, Restricted Project

Nov 13 2020

rjmccall accepted D91471: [Coroutines] Make sure that async coroutine context size is a multiple of the alignment requirement.

LGTM

Nov 13 2020, 8:18 PM · Restricted Project

Nov 12 2020

rjmccall accepted D90622: clang: Don't assert on no_unique_address fields in @encode().

LGTM

Nov 12 2020, 11:09 AM · Restricted Project

Nov 11 2020

rjmccall accepted D91253: [Matrix] Update mangling to use paramterized vendor ext type syntax..

LGTM.

Nov 11 2020, 11:43 AM · Restricted Project
rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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

Nov 11 2020, 10:38 AM · Restricted Project
rjmccall added a comment to D91098: [coro] Async coroutines: Allow more than 3 arguments in the dispatch function.

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.

Nov 11 2020, 10:23 AM · Restricted Project
rjmccall added a comment to D91098: [coro] Async coroutines: Allow more than 3 arguments in the dispatch function.

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

Nov 11 2020, 10:20 AM · Restricted Project
rjmccall added inline comments to D91253: [Matrix] Update mangling to use paramterized vendor ext type syntax..
Nov 11 2020, 10:13 AM · Restricted Project

Nov 10 2020

rjmccall added a comment to D91098: [coro] Async coroutines: Allow more than 3 arguments in the dispatch function.

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

Nov 10 2020, 10:55 PM · Restricted Project
rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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.

Nov 10 2020, 6:01 PM · Restricted Project
rjmccall added inline comments to D90434: [CodeGen] Correct codegen for self-capturing __block var.
Nov 10 2020, 3:23 PM · Restricted Project

Nov 9 2020

rjmccall added a comment to D90434: [CodeGen] Correct codegen for self-capturing __block var.

This is great work, thank you.

Nov 9 2020, 11:44 PM · Restricted Project
rjmccall added a comment to D91098: [coro] Async coroutines: Allow more than 3 arguments in the dispatch function.

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

Nov 9 2020, 11:24 PM · Restricted Project
rjmccall accepted D91111: [CodeGen] Mark calls to objc_autorelease as tail.
Nov 9 2020, 11:22 PM · Restricted Project
rjmccall added a comment to D91111: [CodeGen] Mark calls to objc_autorelease as tail.

Alright.

Nov 9 2020, 11:22 PM · Restricted Project
rjmccall added a comment to D90612: Start of an llvm.coro.async implementation.

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

Nov 9 2020, 6:29 PM · Restricted Project

Nov 6 2020

rjmccall accepted D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects..

LGTM

Nov 6 2020, 11:53 PM · Restricted Project

Nov 5 2020

rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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?

Nov 5 2020, 11:44 AM · Restricted Project

Nov 3 2020

rjmccall added a comment to D90612: Start of an llvm.coro.async implementation.

Looks good to me as a first pass.

Nov 3 2020, 11:46 AM · Restricted Project
rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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.

Nov 3 2020, 10:13 AM · Restricted Project

Nov 2 2020

rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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

Nov 2 2020, 9:36 PM · Restricted Project
rjmccall added a comment to D83448: [CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators.

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

Nov 2 2020, 12:56 PM · Restricted Project
rjmccall accepted D90550: [Coroutines][Docs] Remove frame packing as a TODO.

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

Nov 2 2020, 11:11 AM · Restricted Project

Oct 30 2020

rjmccall added a comment to D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic.

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.

Oct 30 2020, 10:57 AM · Restricted Project

Oct 29 2020

rjmccall accepted D89559: PR47372: Fix Lambda invoker calling conventions.

Alright, LGTM.

Oct 29 2020, 2:23 PM · Restricted Project
rjmccall added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 29 2020, 12:16 PM · Restricted Project
rjmccall added a comment to D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic.

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.

Oct 29 2020, 11:54 AM · Restricted Project

Oct 28 2020

rjmccall added a comment to D90336: [Sema] Diagnose annotating `if constexpr` with a likelihood attribute.

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

Oct 28 2020, 2:46 PM · Restricted Project
rjmccall accepted D90258: [InstCombine] Do not introduce bitcasts for swifterror arguments..
Oct 28 2020, 2:39 PM · Restricted Project

Oct 27 2020

rjmccall added inline comments to D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects..
Oct 27 2020, 1:25 PM · Restricted Project
rjmccall added a comment to D90258: [InstCombine] Do not introduce bitcasts for swifterror arguments..

LGTM. Untyped pointers can't come soon enough.

Oct 27 2020, 12:21 PM · Restricted Project
rjmccall added inline comments to D90174: [HIP] Fix regressions due to fp contract change.
Oct 27 2020, 11:26 AM · Restricted Project

Oct 26 2020

rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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

Oct 26 2020, 2:54 PM · Restricted Project
rjmccall added a comment to D90174: [HIP] Fix regressions due to fp contract change.

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

Oct 26 2020, 11:08 AM · Restricted Project

Oct 23 2020

rjmccall added a comment to D71726: Let clang atomic builtins fetch add/sub support floating point types.

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.

Oct 23 2020, 12:28 PM
rjmccall added a comment to D71726: Let clang atomic builtins fetch add/sub support floating point types.

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.

Oct 23 2020, 10:51 AM
rjmccall added inline comments to D89909: [SYCL] Implement SYCL address space attributes handling.
Oct 23 2020, 10:36 AM · Restricted Project, Restricted Project
rjmccall accepted D83448: [CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators.

LGTM.

Oct 23 2020, 10:22 AM · Restricted Project

Oct 22 2020

rjmccall added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

Oh, I see, sorry.

Oct 22 2020, 9:25 PM · Restricted Project
rjmccall added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

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.

Oct 22 2020, 9:14 AM · Restricted Project

Oct 21 2020

rjmccall added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

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.

Oct 21 2020, 4:11 PM · Restricted Project
rjmccall added a comment to D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var.

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.

Oct 21 2020, 4:01 PM · Restricted Project
rjmccall added a comment to D89768: [Coroutine] Properly determine whether an alloca should live on the frame.

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

Oct 21 2020, 12:20 PM · Restricted Project
rjmccall added a comment to D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var.

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.

Oct 21 2020, 11:56 AM · Restricted Project
rjmccall added a comment to D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var.

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.

Oct 21 2020, 11:54 AM · Restricted Project

Oct 19 2020

rjmccall accepted D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic.

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

Oct 19 2020, 10:59 AM · Restricted Project
rjmccall added inline comments to D89711: [Coroutine] Prevent value reusing across coroutine suspensions in EarlyCSE and GVN.
Oct 19 2020, 10:49 AM · Restricted Project
rjmccall added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

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.

Oct 19 2020, 10:02 AM · Restricted Project

Oct 16 2020

rjmccall added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

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.

Oct 16 2020, 5:11 PM · Restricted Project
rjmccall accepted D89360: Treat constant contexts as being in the default rounding mode..

Okay, thanks. This LGTM.

Oct 16 2020, 12:43 PM · Restricted Project
rjmccall added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

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.

Oct 16 2020, 11:52 AM · Restricted Project
rjmccall added inline comments to D89559: PR47372: Fix Lambda invoker calling conventions.
Oct 16 2020, 10:17 AM · Restricted Project
rjmccall added a comment to D89559: PR47372: Fix Lambda invoker calling conventions.

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

Oct 16 2020, 10:15 AM · Restricted Project
rjmccall added a comment to D89544: [CodingStandards] Clarify the recommendation to use SmallVector.

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.

Oct 16 2020, 10:01 AM · Restricted Project
rjmccall added a comment to D89544: [CodingStandards] Clarify the recommendation to use SmallVector.

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.

Oct 16 2020, 9:55 AM · Restricted Project

Oct 15 2020

rjmccall accepted D89475: [SemaObjC] Fix composite pointer type calculation for `void*` and pointer to lifetime qualified ObjC pointer type.

LGTM

Oct 15 2020, 10:50 AM · Restricted Project
rjmccall added inline comments to D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic.
Oct 15 2020, 10:47 AM · Restricted Project