Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

rjmccall accepted D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws.

Minor suggestion in the docs, but otherwise LGTM.

Wed, Dec 8, 11:45 AM · Restricted Project, Restricted Project
rjmccall accepted D115222: [Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish).

Okay, that's fine, I have no objection to removing it. LGTM.

Wed, Dec 8, 11:36 AM · Restricted Project, Restricted Project

Yesterday

rjmccall added a comment to D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws.

Okay. Well, I'm glad it works. I guess I find it a little strange that coro.end doesn't already mark the coroutine done. I guess the normal C++ lowering always generates a final suspension before reaching a non-unwind coro.end?

Yes, there is always a final suspend before coro.end in normal path. So that the coroutine would be marked done all the time except unhandled_exception throws.

Tue, Dec 7, 11:22 PM · Restricted Project, Restricted Project
rjmccall added a comment to D112616: Fix crash on invalid code involving late parsed inline methods.

Okay. I guess the only choice, then, would be to not try to parse a template deduction guide when you're in a context where template deduction guides aren't allowed. Maybe that's not actually reasonable.

Tue, Dec 7, 8:29 PM · Restricted Project
rjmccall added a comment to D114732: [clang] Mark `trivial_abi` types as "trivially relocatable"..

Trivial relocation doesn't imply that types have to be safe against being suddenly relocated during the middle of operations while they're not in a safe internal state. That is not a consideration.

Tue, Dec 7, 6:05 PM · Restricted Project
rjmccall added a comment to D114732: [clang] Mark `trivial_abi` types as "trivially relocatable"..

I am perfectly happy accepting that syllogism. Passing a value in registers is a trivial relocation. If there is a reason your type shouldn't be trivially relocated, you should not make it trivial_abi.

Tue, Dec 7, 4:36 PM · Restricted Project
rjmccall added a comment to D115222: [Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish).

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

Tue, Dec 7, 11:31 AM · Restricted Project, Restricted Project
rjmccall added a comment to D112616: Fix crash on invalid code involving late parsed inline methods.

The only alternative to this that I can see would be for the invalid code to produce more valid-seeming AST.

Tue, Dec 7, 11:25 AM · Restricted Project
rjmccall added a comment to D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws.

I agree that you shouldn't call suspend, but doesn't coro.end have the behavior of marking the coroutine done? Should we just be calling coro.end on this path?

@rjmccall great insight! coro.end wouldn't marking the coroutine done previously. But its place is perfect to do so. I have added the codes to mark the coroutine as done for coro.end in the unwind path. And I have checked the behavior. The exception happened earlier wouldn't run into the path of coro.end exists. So the behavior is satisfying. The only defect might be that the behavior is C++'s semantic. Although, there might be a problem if there is another language also uses switch-resumed ABI one day. I think the current approach is much better at least it has and would generate less code.

Tue, Dec 7, 11:20 AM · Restricted Project, Restricted Project

Mon, Dec 6

rjmccall added a comment to D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws.

I agree that you shouldn't call suspend, but doesn't coro.end have the behavior of marking the coroutine done? Should we just be calling coro.end on this path?

Mon, Dec 6, 11:48 PM · Restricted Project, Restricted Project
rjmccall added a reviewer for D115222: [Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish): GorNishanov.

I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the intrinsic in the frontend and then (2) checking if the copied parameter variable is actually used after reaching a suspend point, other than in ways that wouldn't happen if the intrinsic returned false.

Mon, Dec 6, 11:33 PM · Restricted Project, Restricted Project
rjmccall added a comment to D114732: [clang] Mark `trivial_abi` types as "trivially relocatable"..

Don't worry about GC::Strong. ObjC GC is thoroughly dead (unless there's GNU-runtime interest in it?), and AFAIK we've never made any effort to incorporate it into our treatment of non-trivial structs.

Mon, Dec 6, 7:04 PM · Restricted Project

Sun, Dec 5

rjmccall added a comment to D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks.

That's not an unreasonable idea, and we did consider it, and in fact it's still on the table as something we could do. However, it's a significantly more complex change, and we're not committed to doing it, and so we wanted to at least put this warning in place to help people avoid a relatively common bug.

Sun, Dec 5, 1:53 PM

Thu, Dec 2

rjmccall accepted D112113: [ObjC] type method metadata `_imp`, messenger routine at callsite with program address space.

Well, I don't envy you the amount of work you're going to have to do to get ObjC working on a Harvard architecture, but this patch LGTM as progress.

Thu, Dec 2, 8:26 PM · Restricted Project
rjmccall accepted D108479: [Clang] Add __builtin_addressof_nocfi.

This looks great, thanks. Please feel free to commit with the requested minor change to the docs.

Thu, Dec 2, 8:20 PM · Restricted Project
rjmccall added a comment to D108643: Introduce _BitInt, deprecate _ExtInt.

I'm sorry for holding this up for so long by just not responding to your pings. Yes, I have no objection to you going forward with this patch, since we're still explicitly saying that it's not yet ABI-stable.

Thu, Dec 2, 8:15 PM · Restricted Project, Restricted Project
rjmccall accepted D114937: [PowerPC] [Clang] Fix alignment adjustment of single-elemented float128.

LGTM

Thu, Dec 2, 2:01 PM · Restricted Project
rjmccall added a comment to D112626: Convert float to double on __builtin_dump_struct.

Hey all! Thanks for taking the time to review my patch and writing the Compiler Explorer examples and everything. I had no idea this was the essentially the wrong approach to this, I'd be happy to do a bigger overhaul of the whole builtin if that would make it more correct, but as Aaron points out I'm very new to this project (and C++ too) and essentially clueless as to how to do that, I submitted this patch because it looked like it was simple enough to issue the fpext to get the float promoted.
If you give me some pointers I'd be more than happy to give it a shot, I should have time in the coming weeks. As a seasoned printf debugger 😄 this builtin is pretty useful to me and I'd like to fix it rather than deprecating or otherwise removing it.

Thu, Dec 2, 12:34 PM · Restricted Project
rjmccall added inline comments to D112626: Convert float to double on __builtin_dump_struct.
Thu, Dec 2, 10:42 AM · Restricted Project

Wed, Dec 1

rjmccall added inline comments to D112626: Convert float to double on __builtin_dump_struct.
Wed, Dec 1, 6:17 PM · Restricted Project
rjmccall added inline comments to D111566: [SYCL] Fix function pointer address space.
Wed, Dec 1, 6:09 PM
rjmccall added inline comments to D114902: [Attrs] Elaborate on the trivial_abi documentation.
Wed, Dec 1, 5:03 PM · Restricted Project

Tue, Nov 30

rjmccall added a comment to D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang.

Those all look good, thanks.

Tue, Nov 30, 11:01 AM · Restricted Project

Mon, Nov 29

rjmccall added a comment to D114692: [CodeGen] Change the linkage of typeinfo object written out with vtable.

Your example is an obvious ODR violation. You've got two totally different classes with external linkage with the same name and namespace. If this somehow isn't blowing up in GCC, I'd be very surprised; it might build, but if these are linked into the same image, the v-table for one of these classes is going to be replaced by the v-table for the other, and you will almost certainly get incorrect dynamic behavior.

Mon, Nov 29, 9:47 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

Thanks, this is exactly what I was looking for. Just some straightforward style/design requests from here.

Mon, Nov 29, 9:28 PM · Restricted Project
rjmccall requested changes to D114692: [CodeGen] Change the linkage of typeinfo object written out with vtable.
Mon, Nov 29, 9:02 PM · Restricted Project
rjmccall requested changes to D114728: [Coroutine] Remove the prologue data of `-fsanitize=function` for split functions.

I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from -fsanitize=functions if it wants to sanitize the resumption calls, which I guess it currently doesn't. So something here may be a necessary fix.

Mon, Nov 29, 12:50 PM · Restricted Project, Restricted Project

Wed, Nov 24

rjmccall added inline comments to D113925: [HIP] Add HIP scope atomic operations.
Wed, Nov 24, 6:49 PM · Restricted Project
rjmccall updated subscribers of D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang.

If you aren't sure what a comment means, please feel free to CC Richard or me, and we might be able to help.

Wed, Nov 24, 6:48 PM · Restricted Project
rjmccall added inline comments to D113925: [HIP] Add HIP scope atomic operations.
Wed, Nov 24, 1:32 PM · Restricted Project

Tue, Nov 23

rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

Your builtin is using custom type-checking (t), which suppresses all the normal conversions that happen on expressions. Specifically, it skips lvalue-to-rvalue conversion, so in this example the argument ends up being an l-value reference to a variable rather than an r-value loaded from that variable. In addition to confusing constant evaluation, it would also make this look like an ODR-use of the variable, which would be subtly wrong in some C++ cases. The fix is to explicitly request the standard l-value conversions, which you can do with code like:

Tue, Nov 23, 6:26 PM · Restricted Project
rjmccall added inline comments to D108479: [Clang] Add __builtin_addressof_nocfi.
Tue, Nov 23, 1:11 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

I worked around this for now by explicitly allowing __builtin_function_start in CheckLValueConstantExpression, but this seems terribly hacky. What would be the correct way to solve this issue?

Tue, Nov 23, 10:19 AM · Restricted Project

Sun, Nov 21

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

Platforms are permitted to make stronger guarantees than the C standard requires, and it is totally reasonable for compilers to assume that malloc meets the target platform's documented guarantees. Arguably, libraries should not be replacing malloc if they fail to meet the platform's documented guarantees.

Sun, Nov 21, 7:58 PM · Restricted Project

Fri, Nov 19

rjmccall accepted D114108: [NFC][clang] Inclusive language: rename master variable to controller in debug-info tests.
Fri, Nov 19, 8:58 AM · Restricted Project

Thu, Nov 18

rjmccall accepted D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag.

Thanks, LGTM

Thu, Nov 18, 10:35 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

If we do need to support constant expressions of this

Yes, we need this also in constant expressions.

Thu, Nov 18, 10:15 AM · Restricted Project
rjmccall accepted D113709: Don't consider `LinkageSpec` when calculating DeclContext `Encloses`.

We don't properly handle lookup through using declarations when there is a linkage spec in the common chain.

Thu, Nov 18, 8:48 AM · Restricted Project
rjmccall requested changes to D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag.
Thu, Nov 18, 8:37 AM · Restricted Project

Wed, Nov 17

rjmccall added inline comments to D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag.
Wed, Nov 17, 6:23 PM · Restricted Project
rjmccall added inline comments to D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag.
Wed, Nov 17, 4:03 PM · Restricted Project

Mon, Nov 15

rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

If we do need to support constant expressions of this, I think we should have the constant evaluator produce an expression rather than an l-value, the way we do with some other builtin calls. That should stop the comparison problem more generally.

Mon, Nov 15, 6:15 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

Do any of the proposed use cases actually require this to be a constant expression? Some of these patterns can be cheaply implemented with code generation even if the target doesn't otherwise support constants; for example, we could just mask the THUMB bit off on 32-bit ARM.

Mon, Nov 15, 6:12 PM · Restricted Project

Sat, Nov 13

rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

For CHERI there's the added complication that descriptors and trampolines can exist for security reasons when crossing security domains, and you absolutely should not let one compartment get pointers to the entry point of another compartment's function. You can hand it out if sealed or the permissions are cleared, as then you can't really do anything with it other than look at the integer address, but that seems a bit odd.

Sat, Nov 13, 8:47 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

I don't know for certain, but I would guess that the kernel wants to get the address of the first instruction in the function for the purposes of some sort of later PC-based table lookup, which means that yes, it probably *does* need to bypass descriptors on CHERI / Itanium / whatever else.

Sat, Nov 13, 7:42 PM · Restricted Project

Fri, Nov 12

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

Let's not bring back the weak function thunk approach. That approach caused problems described in my commit, D8467, which is why the code was removed.

Fri, Nov 12, 9:54 AM · Restricted Project, Restricted Project, Restricted Project
rjmccall requested changes to D112921: [clang] Enable sized deallocation by default in C++14 onwards.

Yeah, I think this either needs deployment restriction on Apple platforms or it needs the thunk / weak-linking approach from the original patch when the target OS isn't recent enough. @ldionne should know the right OS versions.

Fri, Nov 12, 9:23 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Nov 11

rjmccall added a comment to D8467: C++14: Disable sized deallocation by default due to ABI breakage.
In D8467#3125471, @rnk wrote:

Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.

It is already a flag, -fsized-deallocation. On some level, whatever default we choose is just a guess about the C++ library support level. Clang tries to encode all kinds of Linux distro-specific knowledge, and it's often wrong anyway. After all that logic, there will be some default. I think at this point the feature should default to being enabled.

Thu, Nov 11, 12:49 PM
rjmccall added a comment to D8467: C++14: Disable sized deallocation by default due to ABI breakage.

Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basically down to what C++ libraries you've got installed — and probably has to be controlled by a flag. Enabling that flag by default on any particular Linux distribution release is something I'm not sure we can do unconditionally.

Thu, Nov 11, 11:23 AM

Nov 8 2021

rjmccall added a comment to D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default.

Hmm. I wonder if that's related to the problem uncovered by the verifier in https://reviews.llvm.org/D113352.

Nov 8 2021, 10:04 AM · Restricted Project

Nov 7 2021

rjmccall accepted D113352: [clang] Run LLVM Verifier in modes without CodeGen too.

LGTM

Nov 7 2021, 11:24 PM · Restricted Project
rjmccall added inline comments to D113352: [clang] Run LLVM Verifier in modes without CodeGen too.
Nov 7 2021, 8:16 PM · Restricted Project

Nov 6 2021

rjmccall added a comment to D113352: [clang] Run LLVM Verifier in modes without CodeGen too.

Seems like a good idea to me. Minor request; otherwise, feel free to commit.

Nov 6 2021, 11:01 PM · Restricted Project

Nov 5 2021

rjmccall added inline comments to D112941: [clang] Add support for the new pointer authentication builtins..
Nov 5 2021, 11:02 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

(Adding back @rsmith, @rjmccall.)

You could also make this Just Work for things like C++ member functions rather than producing a member function pointer.

I'm not sure I understand. What does Just Work mean when it comes to C++ member functions?

I think he means that e.g. __builtin_symbol_address(&Foo::bar) should return a void* pointing to the address of the Foo::bar member function body, instead of a member function pointer for Foo::bar.

Nov 5 2021, 6:54 PM · Restricted Project
rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

Yeah, I think that's a better name. The documentation can say that ideally this also wouldn't include things like the THUMB bit, but there are technical limitations that make it hard to achieve that.

Nov 5 2021, 12:22 PM · Restricted Project

Nov 4 2021

rjmccall added a comment to D108479: [Clang] Add __builtin_addressof_nocfi.

std::addressof(&someFunction) certainly ought to return a signed pointer under ptrauth, so if your goal here is to get a completely unadorned symbol address, I think you do need a different builtin, and it probably ought to return a void* to emphasize that it shouldn't be used as a normal value. Maybe it should even be semantically restricted to require a constant decl reference as its operand? Related and perhaps illuminating question: if it were implementable, would you also want to force the suppression of lazy-binding thunks and/or decorations like the THUMB bit?

Nov 4 2021, 3:00 PM · Restricted Project

Nov 3 2021

rjmccall accepted D108122: FunctionAttrs: do not mark coroutines with odd return path `noreturn`.

LGTM.

Nov 3 2021, 11:05 AM · Restricted Project

Nov 2 2021

rjmccall added a comment to D112941: [clang] Add support for the new pointer authentication builtins..

Mostly LGTM, although I am not the most unbiased reviewer. :)

Nov 2 2021, 9:21 PM · Restricted Project

Nov 1 2021

rjmccall added a comment to D109950: [Clang] Enable IC/IF mode for __ibm128.

Thanks. @hubert.reinterpretcast, @qiucf, can you verify that other compilers for PPC follow the logic for TF / TC that we now have with Elizabeth's patch? There are three different type specifiers (long double, __ibm128, and float128_t) which represent formally distinct types, and IIUC there are a bunch of different flags and target options that change the meaning of long double and/or disable/enable __ibm128 and float128_t. We care about exactly which type is produced by the mode attribute, so you'll have to do something which is sensitive to the exact formal type, like _Generic or a C++ template or doing a pointer conversion without a cast; checking code generation will only tell us the underlying format.

Nov 1 2021, 6:46 PM · Restricted Project
rjmccall accepted D112975: Fix complex types declared using mode TC.

For posterity in case someone tracks down this review: TC corresponds to an unspecified 128-bit format, which on some targets is a double-double format (like __ibm128_t) and on others is float128_t. The bug in the previous patch is that long double is only safe to use when it's known to be one of those formats.

Nov 1 2021, 6:33 PM · Restricted Project

Oct 30 2021

rjmccall added a comment to D90868: [IR] Define @llvm.ptrauth intrinsics..

Well, we've been using ptrauth as a keyword/prefix/etc. to write software at Apple for more than four years, and we have tens of thousands of lines of code using that scattered across several dozen projects, which is probably at least half of the pointer authentication code in the world. I did specifically reach out to the ARM ELF group in the early days of that effort asking that they standardize on ptrauth instead of introducing yet another abbreviation for the extension, but they'd already made a file with "pauth" in the name, so now for better or worse I think we're stuck with having yet another abbreviation for the extension.

Oct 30 2021, 9:36 PM · Restricted Project
rjmccall added a comment to D108741: [IR][GlobalDCE] Extend !vcall_visibility to be able to specify an offset range that it applies to.

I think Peter is saying that if you can make Clang emit correct ranges for v-tables — specifically so that the ranges don't cover the type_info pointer, which we don't emit enough metadata to safely remove — then you can also go ahead and remove the special case where we only eliminate loads of function pointers. That's assuming we don't need to support old bitcode.

Oct 30 2021, 2:50 PM · Restricted Project

Oct 29 2021

rjmccall added a comment to D109950: [Clang] Enable IC/IF mode for __ibm128.

Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?

I think the old logic should be preserved yes. However, I'm not sure if this patch exposes an existing bug. LongDoubleFormat here is llvm::semX87DoubleExtended.

In APFloat.cpp, it is defined as -

static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};

i.e. 80 bit size. Is this the right format for complex type specified using mode 'TC'? I am not very familiar with floating point semantics, so I thought I would ask.

Oct 29 2021, 1:33 PM · Restricted Project
rjmccall added a comment to D109950: [Clang] Enable IC/IF mode for __ibm128.

Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right?

Oct 29 2021, 10:31 AM · Restricted Project

Oct 26 2021

rjmccall requested changes to D111669: No longer crash when a consteval function returns a structure.

Hmm. Generally these cases are expected to handle the situation where there's no result slot passed in, which currently isn't exclusive to an ignored result (although you could argue it should be). IIRC we usually don't pass in a slot when evaluating an expression of agg type as anything except an initializer, e.g. when evaluating the E in E.member. The general fix is to call EnsureDest before accessing Dest. The only reason we wouldn't need to do that is if ConstantExpr is restricted in where it can appear such that that's impossible.

Oct 26 2021, 4:31 PM · Restricted Project
rjmccall accepted D111814: Fix consteval crash when transforming 'this' expressions.

This seems reasonable. You could also give ExitFunctionBodyRAII an explicit pop() operation, but either way is fine.

Oct 26 2021, 4:14 PM · Restricted Project

Oct 23 2021

rjmccall accepted D112235: [HIP][OpenMP] Fix assertion in deferred diag due to incomplete class definition.

Minor grammar nit with a comment, but otherwise LGTM.

Oct 23 2021, 10:58 PM · Restricted Project

Oct 22 2021

rjmccall accepted D108482: [Clang] Fix instantiation of OpaqueValueExprs (Bug #45964).

LGTM, then.

Oct 22 2021, 11:38 AM · Restricted Project

Oct 20 2021

rjmccall added a comment to D108122: FunctionAttrs: do not mark coroutines with odd return path `noreturn`.

Well, I think the basic problem is that unlowered coroutines cannot be modeled as normal functions very well in LLVM IR, and the coro intrinsics are doing their best to muddle through given the unstated requirement to not bother anyone else too much. We have major representational problems with modeling some of the things we need, especially the distinctions between ordinary-ish behavior in the ramp function, logically separate from the coroutine, vs. things that are semantically part of the coroutine body.

Oct 20 2021, 11:09 PM · Restricted Project
rjmccall added inline comments to D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation.
Oct 20 2021, 11:52 AM · Restricted Project

Oct 19 2021

rjmccall added a comment to D108741: [IR][GlobalDCE] Extend !vcall_visibility to be able to specify an offset range that it applies to.

@rjmccall, please correct me where I'm wrong :)

Oct 19 2021, 1:05 PM · Restricted Project
rjmccall added a comment to D112081: Define __STDC_NO_THREADS__ when targeting windows-msvc (PR48704).

The standard answer is that compilers are designed to work with a specific set of system headers. In the Clang-on-Windows case, that's complicated by the fact that many people acquire Clang separately from the rest of the build environment (although Microsoft does distribute Clang officially now, right?), but I think the standard answer is still ultimately the correct one: Clang is designed to support the MSVC system headers that are currently out in the world, and whenever Microsoft releases new system headers, it's possible that you will need a new version of Clang.

Oct 19 2021, 11:04 AM · Restricted Project

Oct 18 2021

rjmccall accepted D91815: Fix small typo in Block ABI docs.

LGTM

Oct 18 2021, 1:53 PM · Restricted Project

Oct 15 2021

rjmccall added a comment to D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation.

I don't think we currently hold very strongly to that ActOn vs. Build philosophy across all productions, but it does seem like a good idea.

Oct 15 2021, 3:10 PM · Restricted Project

Oct 12 2021

rjmccall added a comment to D108643: Introduce _BitInt, deprecate _ExtInt.

Yes, I think it would be fine to fork off a conversation about what the ABI should be; I didn't meant to completely derail this patch. To be frank, my expectation was that more of this was already settled and documented, so we just needed to add that documentation and clean up a few details.

Oct 12 2021, 11:04 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D108643: Introduce _BitInt, deprecate _ExtInt.
Oct 12 2021, 1:55 AM · Restricted Project, Restricted Project
rjmccall added a comment to D108643: Introduce _BitInt, deprecate _ExtInt.

! In D108643#3054443, @rjmccall wrote:

! In D108643#3027473, @erichkeane wrote:

! In D108643#3026550, @rjmccall wrote:

I think it would be better to provide a generic ABI specification that is designed to "lower" _BitInt types into more basic concepts that ABIs typically already have rules for. It would be appropriate to note at the top that this is only a generic specification and that the rules for any particular platform should be codified in a platform-specific ABI. But we should make that easy so that platform owners who don't have strong opinions about the ABI can just say "as described in version 1.4 of the generic ABI at https://clang.llvm.org/..."

Now, that is all independent of what we actually decide the ABI should be. But I do think that if we're going to make this available on all targets, we should strongly consider doing a "legalizing" lowering in the frontend: at ABI-exposed points (call arguments/parameters and memory accesses), _BitInt types should be translated into types that a naive backend can be trusted to handle in a way that matches the documented ABI. Individual targets can then opt in to using the natural lowerings if they promise to match the ABI, but that becomes a decision that they make to enable better optimization and code generation, not something that's necessary for correctness.

Don't we already have that? We work the same way a 'bool' does, that is, we zero/sign extend for parameters and return values? The backends then reliably up-scale these to the power-of-two/multiple-of-pointer.

There's two levels of a "legalizing" lowering.

On the specification level, we'd be formally saying that the ABI matches the ABI of some other C construct. For example, we might say that a signed _BitInt(14) argument is treated exactly as if the argument type was instead signed short (with whatever restriction we do or do not impose on the range). This is very nice on this level because, first, it's a rule that works generically for all targets without needing to care about the details of how arguments are assigned to registers or stack slots or whatever; and second, it means you're never adding new cases to the ABI that e.g. libffi would need to care about; and third, you can reliably match the ABI with manually-lowered C code from a compiler that doesn't even support _BitInts.

On the implementation level, we'd have to actually emit IR which will turn into code which matches that ABI. I think you are right that small integers already reliably work that way in LLVM, so that if you pass an i11 sext it'll get sign-extended to some width — what width exactly, I don't know, but probably the same width that i16 sext would be. Larger integers, I'm not sure. A legalizing lowering of big _BitInt argument would presumably say either that it's exactly the same as a struct containing all the chunks or that it's exactly the same as a series of arguments for each of the chunks. Those are two very different rules! Most ABIs won't "straddle" a single argument between registers and the stack, so e.g. if you'd need two registers and you only have one left then you exhaust the registers and go fully on the stack; but obviously you can get straddling if the lowering expands the sequence. Similarly, some ABIs will go indirect if the struct gets big enough, but you won't get that if the lowering expands. But on the other hand, if the rule is really that it's like a struct, well, some ABIs don't pass small structs in registers. Anyway, my point is that what exactly LLVM targets do if you give them an i117 might not match either of these possibilities reliably across targets. In any case, since we currently have to count registers in the frontend for the register-passing ABIs, we'll need to actually know what the targets do.

Hmm... now that I think I understand the concern, I'm not sure we're able to take on this level of commitment. The Clang ABI code is a bit of a mess in general, plus each target has its own code, right (In CodeGen/TargetInfo.cpp)?

Oct 12 2021, 12:39 AM · Restricted Project, Restricted Project
rjmccall added a comment to D111334: [ObjC][ARC] Handle operand bundle "clang.arc.attachedcall" on targets that don't use the inline asm marker.

Ok. I'm not an expert in this code, but it seems fine to me.

Oct 12 2021, 12:06 AM · Restricted Project
rjmccall added a comment to D108741: [IR][GlobalDCE] Extend !vcall_visibility to be able to specify an offset range that it applies to.

Okay. Thanks for the explanation, I think that was helpful.

Oct 12 2021, 12:02 AM · Restricted Project

Oct 11 2021

rjmccall accepted D111331: [ObjC][ARC] Use operand bundle "clang.arc.attachedcall" on x86-64.

Thanks, LGTM.

Oct 11 2021, 10:50 PM · Restricted Project
rjmccall added inline comments to D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas.
Oct 11 2021, 10:49 PM · Restricted Project
rjmccall added a comment to D108741: [IR][GlobalDCE] Extend !vcall_visibility to be able to specify an offset range that it applies to.

I did skim the thread, and it seemed to me that Peter's objection was largely to the reuse of !type, which he feels wasn't meant to carry these semantics. Peter leapt to the idea of a new constant instead of a more general metadata, but I don't understand what that leap is meant to achieve, which is why I'm engaging with the thread.

Oct 11 2021, 4:34 PM · Restricted Project
rjmccall added a comment to D108741: [IR][GlobalDCE] Extend !vcall_visibility to be able to specify an offset range that it applies to.

I don't entirely understand the purpose of vfe_eligible. It sounds like it expresses that it's okay for this value to be replaced if we can prove that nothing ever uses it. Surely that's universally true, albeit usually extremely hard to prove. So maybe the right general approach is that we should be saying something about the v-table object: that we know certain fields in it are only accessed in specially blessed patterns, and then this !type metadata or whatever lets us prove that certain patterns don't exist. I don't know that a new constant is the best way to express that, as opposed to some sort of metadata on the v-table. At the very least, maybe that gives us some direction towards a more general name for the constant.

Oct 11 2021, 2:43 PM · Restricted Project
rjmccall accepted D111316: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca().

Okay, thanks, Alexey. LGTM, then.

Oct 11 2021, 2:20 PM · Restricted Project
rjmccall added a comment to D108643: Introduce _BitInt, deprecate _ExtInt.

I think it would be better to provide a generic ABI specification that is designed to "lower" _BitInt types into more basic concepts that ABIs typically already have rules for. It would be appropriate to note at the top that this is only a generic specification and that the rules for any particular platform should be codified in a platform-specific ABI. But we should make that easy so that platform owners who don't have strong opinions about the ABI can just say "as described in version 1.4 of the generic ABI at https://clang.llvm.org/..."

Now, that is all independent of what we actually decide the ABI should be. But I do think that if we're going to make this available on all targets, we should strongly consider doing a "legalizing" lowering in the frontend: at ABI-exposed points (call arguments/parameters and memory accesses), _BitInt types should be translated into types that a naive backend can be trusted to handle in a way that matches the documented ABI. Individual targets can then opt in to using the natural lowerings if they promise to match the ABI, but that becomes a decision that they make to enable better optimization and code generation, not something that's necessary for correctness.

Don't we already have that? We work the same way a 'bool' does, that is, we zero/sign extend for parameters and return values? The backends then reliably up-scale these to the power-of-two/multiple-of-pointer.

Oct 11 2021, 1:54 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D108643: Introduce _BitInt, deprecate _ExtInt.
Oct 11 2021, 1:18 AM · Restricted Project, Restricted Project
rjmccall accepted D109950: [Clang] Enable IC/IF mode for __ibm128.

Thanks, LGTM

Oct 11 2021, 12:17 AM · Restricted Project
rjmccall added a reviewer for D111316: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca(): ABataev.

This mostly looks good, but I'd like Alexey Bataev to sign off, since he authored the OpenMP support.

Oct 11 2021, 12:17 AM · Restricted Project

Oct 8 2021

rjmccall accepted D109948: [Clang] Enable _Complex __ibm128 type.

LGTM

Oct 8 2021, 12:20 PM · Restricted Project
rjmccall accepted D111324: [CFE][Codegen] Remove CodeGenFunction::InitTempAlloca().

Thanks. This LGTM when all the patches it depends on are checked in.

Oct 8 2021, 12:19 PM · Restricted Project
rjmccall added a comment to D109950: [Clang] Enable IC/IF mode for __ibm128.

I agree. It doesn't have to be a CodeGen test, just anything that directly verifies that we get the right type, since I think those calls can succeed due to promotion.

Oct 8 2021, 12:18 PM · Restricted Project
rjmccall accepted D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca().

Patch LGTM, but please remove the test, it's been folded into a test I added in my patch.

Oct 8 2021, 10:52 AM · Restricted Project
rjmccall added a comment to D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca().

Hmm. I decided the ObjC case needed a more complex change and went ahead and committed that fix here: https://github.com/llvm/llvm-project/commit/5ab6ee75994d. That obviates the need for the new test; sorry for the runaround. Please rebase and I'll review.

Oct 8 2021, 2:46 AM · Restricted Project
rjmccall committed rG5ab6ee75994d: Fix a variety of bugs with nil-receiver checks when targeting (authored by rjmccall).
Fix a variety of bugs with nil-receiver checks when targeting
Oct 8 2021, 2:44 AM

Oct 7 2021

rjmccall added a comment to D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca().

CodeGenFunction::InitTempAlloca() inits the static alloca within the entry block which may *not* necessarily be correct always.

FWIW, for all uses this was correct. The point of the function was exactly to do what you state here as "potentially incorrect".
The documentation explicitly states "and the initializer must be valid in the entry block (i.e. it must either be a constant or an argument value)."
The rest of the reasoning that follows for this patch is consequently mood.

While I don't expect this to have a real negative impact on performance it certainly could if we are somewhat sensitive to the additional stores that
are now executed prior to an event which requires a temporary storage location (in OpenMP).

Oct 7 2021, 4:33 PM · Restricted Project
rjmccall added a comment to D111334: [ObjC][ARC] Handle operand bundle "clang.arc.attachedcall" on targets that don't use the inline asm marker.

Okay, so does this mean that this works on arbitrary targets now? If so, should Clang be using it unconditionally?

Oct 7 2021, 2:40 PM · Restricted Project
rjmccall added inline comments to D111331: [ObjC][ARC] Use operand bundle "clang.arc.attachedcall" on x86-64.
Oct 7 2021, 2:39 PM · Restricted Project
rjmccall added a comment to D111324: [CFE][Codegen] Remove CodeGenFunction::InitTempAlloca().

Can you just do one patch for all the OpenMP changes, and then put the final removal in a separate patch?

Oct 7 2021, 2:37 PM · Restricted Project
rjmccall added a comment to D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca().

We should take the opportunity to add a test here for the ObjC case. It should be testable with something like this, where you should see the store inside of the loop instead of once in the prologue:

Oct 7 2021, 2:34 PM · Restricted Project