Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (492 w, 5 d)

Recent Activity

Yesterday

rjmccall added inline comments to D123952: [FPEnv] Allow CompoundStmt to keep FP options.
Wed, Jun 29, 8:55 AM · Restricted Project, Restricted Project

Tue, Jun 28

rjmccall added a comment to D123952: [FPEnv] Allow CompoundStmt to keep FP options.

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

Tue, Jun 28, 11:28 AM · Restricted Project, Restricted Project

Mon, Jun 27

rjmccall added a comment to D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible.

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

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

Mon, Jun 27, 6:25 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D123952: [FPEnv] Allow CompoundStmt to keep FP options.
Mon, Jun 27, 2:49 PM · Restricted Project, Restricted Project
rjmccall added a comment to D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible.

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

Mon, Jun 27, 2:38 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D123952: [FPEnv] Allow CompoundStmt to keep FP options.
Mon, Jun 27, 8:45 AM · Restricted Project, Restricted Project

Sun, Jun 26

rjmccall accepted D128571: [X86] Support `_Float16` on SSE2 and up.

Thank you, LGTM.

Sun, Jun 26, 11:33 PM · Restricted Project, Restricted Project, Restricted Project

Sat, Jun 25

rjmccall added inline comments to D128571: [X86] Support `_Float16` on SSE2 and up.
Sat, Jun 25, 8:47 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Jun 24

rjmccall added inline comments to D123952: [FPEnv] Allow CompoundStmt to keep FP options.
Fri, Jun 24, 7:20 AM · Restricted Project, Restricted Project
rjmccall accepted D128482: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations.

LGTM

Fri, Jun 24, 6:12 AM · Restricted Project, Restricted Project
rjmccall accepted D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible.

Seems right to me.

Fri, Jun 24, 6:10 AM · Restricted Project, Restricted Project

Fri, Jun 17

rjmccall accepted D125936: [Sema] Relax an assertion in BuildStmtExpr.

Okay, LGTM then

Fri, Jun 17, 12:04 PM · Restricted Project, Restricted Project

Thu, Jun 16

rjmccall added inline comments to D125936: [Sema] Relax an assertion in BuildStmtExpr.
Thu, Jun 16, 10:59 AM · Restricted Project, Restricted Project

Mon, Jun 13

rjmccall added a comment to D125936: [Sema] Relax an assertion in BuildStmtExpr.

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

Mon, Jun 13, 11:31 PM · Restricted Project, Restricted Project
rjmccall added a comment to D125349: [clang][sema] Generate builtin operator overloads for (volatile) _Atomic types.

Minor suggestion for a cleanup, then LGTM.

Mon, Jun 13, 11:20 AM · Restricted Project, Restricted Project
rjmccall accepted D127471: [Coroutines] Convert coroutine.presplit to enum attr.

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

Mon, Jun 13, 9:34 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Sat, Jun 11

rjmccall added a comment to D125936: [Sema] Relax an assertion in BuildStmtExpr.

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

Sat, Jun 11, 12:35 PM · Restricted Project, Restricted Project
rjmccall added a comment to D127545: Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference..

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

Sat, Jun 11, 12:28 PM · Restricted Project, Restricted Project

Fri, Jun 10

rjmccall added inline comments to D125349: [clang][sema] Generate builtin operator overloads for (volatile) _Atomic types.
Fri, Jun 10, 6:28 AM · Restricted Project, Restricted Project

Thu, Jun 9

rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

Thu, Jun 9, 8:46 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D125349: [clang][sema] Generate builtin operator overloads for (volatile) _Atomic types.
Thu, Jun 9, 8:24 AM · Restricted Project, Restricted Project
rjmccall accepted D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.

Okay, LGTM

Thu, Jun 9, 7:21 AM · Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

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

Thu, Jun 9, 7:19 AM · Restricted Project, Restricted Project

Wed, Jun 8

rjmccall added a comment to D125349: [clang][sema] Generate builtin operator overloads for (volatile) _Atomic types.

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

Wed, Jun 8, 10:35 AM · Restricted Project, Restricted Project
rjmccall added a comment to D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.

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

Wed, Jun 8, 10:07 AM · Restricted Project, Restricted Project

Tue, Jun 7

rjmccall added inline comments to D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.
Tue, Jun 7, 2:20 PM · Restricted Project, Restricted Project

Mon, Jun 6

rjmccall added inline comments to D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.
Mon, Jun 6, 9:22 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.
Mon, Jun 6, 8:14 AM · Restricted Project, Restricted Project

Fri, Jun 3

rjmccall added a comment to D62574: Add support for target-configurable address spaces..

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

Fri, Jun 3, 9:17 PM · Restricted Project, Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

Fri, Jun 3, 9:10 PM · Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

Fri, Jun 3, 10:22 AM · Restricted Project, Restricted Project

Thu, Jun 2

rjmccall added a comment to D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.

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

Thu, Jun 2, 3:40 PM · Restricted Project, Restricted Project
rjmccall accepted D126715: [coro async] Add code to support dynamic aligment of over-aligned types in async frames.

LGTM

Thu, Jun 2, 2:01 PM · Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

Thu, Jun 2, 1:59 PM · Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

Thu, Jun 2, 12:55 PM · Restricted Project, Restricted Project
rjmccall added a comment to D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder.

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

Thu, Jun 2, 12:37 PM · Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

Thu, Jun 2, 12:31 PM · Restricted Project, Restricted Project
rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

  1. It looks like we're completely dropping qualifiers on the return types of function in C. If that's the case, what's even the point of having qualifiers on return types of functions?
Thu, Jun 2, 12:26 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D126715: [coro async] Add code to support dynamic aligment of over-aligned types in async frames.
Thu, Jun 2, 11:01 AM · Restricted Project, Restricted Project
rjmccall added a comment to D126715: [coro async] Add code to support dynamic aligment of over-aligned types in async frames.

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

Thu, Jun 2, 10:18 AM · Restricted Project, Restricted Project

Tue, May 31

rjmccall added inline comments to D126715: [coro async] Add code to support dynamic aligment of over-aligned types in async frames.
Tue, May 31, 1:02 PM · Restricted Project, Restricted Project

May 19 2022

rjmccall accepted D125635: Make CompoundStmtBitfields::NumStmts not a bit-field.

Thanks, LGTM.

May 19 2022, 7:42 PM · Restricted Project, Restricted Project

May 18 2022

rjmccall added a comment to D125919: Drop qualifiers from return types in C (DR423).

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

May 18 2022, 1:09 PM · Restricted Project, Restricted Project

May 16 2022

rjmccall added a comment to D124435: [X86] Always extend the integer parameters in callee.

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

May 16 2022, 6:14 PM · Restricted Project, Restricted Project

May 15 2022

rjmccall added a comment to D125635: Make CompoundStmtBitfields::NumStmts not a bit-field.

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

May 15 2022, 3:04 PM · Restricted Project, Restricted Project

May 9 2022

rjmccall added inline comments to D124435: [X86] Always extend the integer parameters in callee.
May 9 2022, 4:33 PM · Restricted Project, Restricted Project
rjmccall accepted D124736: [CodeGen] Use ABI alignment for C++ new expressions.

LGTM

May 9 2022, 4:00 PM · Restricted Project, Restricted Project

May 5 2022

rjmccall added a comment to D123134: [demangler] No need to space adjacent template closings.

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

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

May 2 2022

rjmccall added a comment to D124736: [CodeGen] Use ABI alignment for C++ new expressions.

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

May 2 2022, 12:41 AM · Restricted Project, Restricted Project

Apr 30 2022

rjmccall added a comment to D122983: [C11/C2x] Change the behavior of the implicit function declaration warning.

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

Apr 30 2022, 12:55 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Apr 28 2022

rjmccall added a comment to D124211: Add __builtin_kcfi_call_unchecked.

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

Apr 28 2022, 8:11 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D124361: [Coroutines] Add coro_maychange intrinsic to solve TLS problem (2/5).
Apr 28 2022, 4:56 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D124361: [Coroutines] Add coro_maychange intrinsic to solve TLS problem (2/5).
Apr 28 2022, 3:27 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D124571: Avoid strict aliasing violation on type punning inside llvm::PointerIntPair.
Apr 28 2022, 2:59 PM · Restricted Project, Restricted Project

Apr 27 2022

rjmccall added inline comments to D124363: [Coroutines] Don't optimize readnone function before we split coroutine (4/5).
Apr 27 2022, 10:53 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D124435: [X86] Always extend the integer parameters in callee.
Apr 27 2022, 11:02 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D124435: [X86] Always extend the integer parameters in callee.
Apr 27 2022, 2:21 AM · Restricted Project, Restricted Project

Apr 26 2022

rjmccall added inline comments to D124435: [X86] Always extend the integer parameters in callee.
Apr 26 2022, 5:05 PM · Restricted Project, Restricted Project
rjmccall added a comment to D112921: [clang] Enable sized deallocation by default in C++14 onwards.

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

Apr 26 2022, 9:26 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Apr 25 2022

rjmccall added a comment to D112921: [clang] Enable sized deallocation by default in C++14 onwards.

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

Apr 25 2022, 2:51 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Apr 21 2022

rjmccall added a comment to D123510: [clang][WebAssembly] Implement support for table types and appropriate semantic restrictions.

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

Apr 21 2022, 10:07 AM · Restricted Project

Apr 19 2022

rjmccall added a comment to D122963: [X86] Extend the integer parameter if the function isn't local linked.

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

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

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

Apr 19 2022, 11:23 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122963: [X86] Extend the integer parameter if the function isn't local linked.

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

Apr 19 2022, 10:10 PM · Restricted Project, Restricted Project

Apr 18 2022

rjmccall added a comment to D122963: [X86] Extend the integer parameter if the function isn't local linked.

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

Apr 18 2022, 9:40 PM · Restricted Project, Restricted Project
rjmccall accepted D123950: Look through calls to std::addressof to compute pointer alignment..

LGTM

Apr 18 2022, 11:02 AM · Restricted Project, Restricted Project
rjmccall added a comment to D123898: Fix crash in ObjC codegen introduced with 5ab6ee75994d645725264e757d67bbb1c96fb2b6.

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

Apr 18 2022, 10:41 AM · Restricted Project, Restricted Project

Apr 16 2022

rjmccall added a comment to D123898: Fix crash in ObjC codegen introduced with 5ab6ee75994d645725264e757d67bbb1c96fb2b6.

LGTM. Could you add a test case?

Apr 16 2022, 9:41 PM · Restricted Project, Restricted Project

Apr 15 2022

rjmccall added a comment to D123531: [GlobalsModRef][FIX] Ensure we honor synchronizing effects of intrinsics.

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

Apr 15 2022, 11:50 AM · Restricted Project, Restricted Project
rjmccall added a comment to D123531: [GlobalsModRef][FIX] Ensure we honor synchronizing effects of intrinsics.

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

Apr 15 2022, 11:26 AM · Restricted Project, Restricted Project

Apr 14 2022

rjmccall added a comment to D123642: [clang codegen] Assume arguments of __atomic_* are aligned..

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

Apr 14 2022, 2:18 PM · Restricted Project, Restricted Project

Apr 9 2022

rjmccall added a comment to D116203: [clang] adds unary type transformations as compiler built-ins.

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

Apr 9 2022, 12:27 PM · Restricted Project, Restricted Project

Apr 6 2022

rjmccall added a comment to D116203: [clang] adds unary type transformations as compiler built-ins.

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

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

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

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

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

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

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


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

template<class T>
using remove_rcvref_t = __remove_qualifiers(T, const, volatile, restrict, &, &&); // rcv instead of cvr to prevent a typo with remove_cvref_t
Apr 6 2022, 4:50 PM · Restricted Project, Restricted Project

Apr 5 2022

rjmccall added a comment to D116203: [clang] adds unary type transformations as compiler built-ins.

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

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

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

Apr 5 2022, 9:34 PM · Restricted Project, Restricted Project

Apr 1 2022

rjmccall added a comment to D116203: [clang] adds unary type transformations as compiler built-ins.

This patch looks awesome, Chris.

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

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

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

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

Apr 1 2022, 11:07 PM · Restricted Project, Restricted Project
rjmccall accepted D122820: [GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs.
Apr 1 2022, 11:27 AM · Restricted Project, Restricted Project

Mar 31 2022

rjmccall added inline comments to D122820: [GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs.
Mar 31 2022, 1:43 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122820: [GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs.

Unfortunately, this initialization case does allow a situation where

Mar 31 2022, 12:08 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct.

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

Mar 31 2022, 11:59 AM · Restricted Project, Restricted Project
rjmccall accepted D122375: [CoroSplit] Handle argument being the frame pointer.

LGTM

Mar 31 2022, 11:04 AM · Restricted Project, Restricted Project

Mar 30 2022

rjmccall added inline comments to D122375: [CoroSplit] Handle argument being the frame pointer.
Mar 30 2022, 9:48 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122383: [Coroutines] [Retcon] Replace CoroBegin with FramePtr after.

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

Mar 30 2022, 10:01 AM · Restricted Project, Restricted Project

Mar 29 2022

rjmccall added a comment to D122383: [Coroutines] [Retcon] Replace CoroBegin with FramePtr after.

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

Mar 29 2022, 9:36 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types..

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

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

Mar 29 2022, 12:02 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122383: [Coroutines] [Retcon] Replace CoroBegin with FramePtr after.

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

Mar 29 2022, 11:56 AM · Restricted Project, Restricted Project
rjmccall added a comment to D122383: [Coroutines] [Retcon] Replace CoroBegin with FramePtr after.

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

Mar 29 2022, 12:10 AM · Restricted Project, Restricted Project

Mar 28 2022

rjmccall added a comment to D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types..

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

Mar 28 2022, 11:35 AM · Restricted Project, Restricted Project

Mar 25 2022

rjmccall added inline comments to D122460: [clang] fixed bug #54406.
Mar 25 2022, 10:49 PM · Restricted Project, Restricted Project
rjmccall added a comment to D122474: demangler] Parenthesize >> inside template args.

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

Mar 25 2022, 10:12 PM · Restricted Project, Restricted Project, Restricted Project

Mar 24 2022

rjmccall added inline comments to D120905: [demangler] Add operator precedence.
Mar 24 2022, 1:21 PM · Restricted Project, Restricted Project, Restricted Project

Mar 2 2022

rjmccall accepted D120856: [NFC][Clang][OpaquePtr] Remove calls to Address::deprecated in CGBlocks.cpp.

LGTM

Mar 2 2022, 6:31 PM · Restricted Project, Restricted Project

Feb 28 2022

rjmccall added a comment to D118525: [modules] Merge ObjC interface ivars with anonymous types..

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

Feb 28 2022, 11:02 AM · Restricted Project, Restricted Project, Restricted Project

Feb 25 2022

rjmccall added a comment to D120566: [OpenCL][AMDGPU]: Do not allow a call to kernel.

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

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

Feb 25 2022, 3:16 PM · Restricted Project
rjmccall added a comment to D120566: [OpenCL][AMDGPU]: Do not allow a call to kernel.

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

Feb 25 2022, 1:25 PM · Restricted Project

Feb 17 2022

rjmccall added a comment to D119385: Use trivial relocation operations in std::vector, by porting D67524 and part of D61761 to work on top of the changes in D114732..

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

Feb 17 2022, 11:58 PM · Restricted Project, Restricted Project

Feb 12 2022

rjmccall added a comment to D119364: Refactor nested if else with ternary operator in CGExprScalar.cpp.

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

Feb 12 2022, 12:07 PM · Restricted Project

Feb 11 2022

rjmccall added a comment to D119051: Extend the C++03 definition of POD to include defaulted functions.

And this should be raised as an Itanium issue.

Feb 11 2022, 2:23 PM · Restricted Project, Restricted Project
rjmccall added a comment to D119051: Extend the C++03 definition of POD to include defaulted functions.

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

Feb 11 2022, 2:22 PM · Restricted Project, Restricted Project

Feb 10 2022

rjmccall accepted D119364: Refactor nested if else with ternary operator in CGExprScalar.cpp.

Sure, LGTM

Feb 10 2022, 12:42 AM · Restricted Project

Feb 8 2022

rjmccall added inline comments to D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer.
Feb 8 2022, 6:05 PM · Restricted Project, Restricted Project
rjmccall added a comment to D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer.

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

Feb 8 2022, 3:07 PM · Restricted Project, Restricted Project