Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (530 w, 6 d)

Recent Activity

Today

rjmccall added a comment to D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method.

The user isn't modifying the float_t type definition, they're using it. I think the diagnostic should say something like cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit.

Is there any way we can infer an attribute for these typedefs when they're declared, then diagnose it in DiagnoseUseOfDecl? Some sort of "available_only_in_default_eval_method" attribute?

@rjmccall Thanks for the review.
I might be able to call the DiagnoseUseOfDecl here https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L1609 but I don't seem to have access to the NameDecl (first argument of the function).

Thu, Mar 23, 12:01 PM · Restricted Project, Restricted Project

Yesterday

rjmccall added a comment to D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method.

The user isn't modifying the float_t type definition, they're using it. I think the diagnostic should say something like cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit.

Wed, Mar 22, 9:24 AM · Restricted Project, Restricted Project

Tue, Mar 21

rjmccall added a comment to D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds.

Okay. I was looking at a version of the standard that makes reading the environment UB. If that's been relaxed, then I agree that it would be much more natural to talk about *changing* the environment than just *accessing* it.

Tue, Mar 21, 2:06 PM · Restricted Project, Restricted Project

Mon, Mar 20

rjmccall added inline comments to D146386: [MS ABI] Fix mangling references to declarations..
Mon, Mar 20, 5:36 PM · Restricted Project, Restricted Project

Sun, Mar 19

rjmccall added inline comments to D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds.
Sun, Mar 19, 10:20 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds.
Sun, Mar 19, 9:30 PM · Restricted Project, Restricted Project

Thu, Mar 16

rjmccall added inline comments to D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds.
Thu, Mar 16, 12:28 AM · Restricted Project, Restricted Project

Thu, Mar 9

rjmccall added a comment to D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind.

So your argument is that it would not be possible to recognize that we're doing such an optimization and mark the function as having had a possible semantics change?

Thu, Mar 9, 2:10 PM · Restricted Project, Restricted Project
rjmccall added a comment to D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind.
  • At IRGen time, you know the LLVM attributes have not been adjusted after the optimized refined the function's behaviour. It should be safe to have IPA peepholes, as long as IRGen's other peepholes don't refine behaviour and add attributes based on that.
  • In the optimizer, if you're looking at de-refineable function then you don't know which attributes come directly from the source and which were implied by optimizer refinements. You can't trust you'll get the same function attributes at runtime.
Thu, Mar 9, 1:27 PM · Restricted Project, Restricted Project
rjmccall added a comment to D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind.

There are *some* properties we can still assume about linkonce_odr functions despite them being replaceable at link time. The high-level language guarantee we're starting from is that the source semantics of all versions of the function are identical. The version of the function we're looking at has been transformed from the original source — it is, after all, now LLVM IR, not C/C++ — but it has presumably faithfully preserved the source semantics. We can therefore rely on any properties of the semantics that are required to be preserved by transformation, which includes things like "does it terminate", "what value does it return", "what side effects does it perform", and so on. What we can't rely on are properties of the implementation that are not required to be preserved by transformation, like whether or not it uses a certain argument — transformations are permitted to change that.

Thu, Mar 9, 9:15 AM · Restricted Project, Restricted Project

Mon, Mar 6

rjmccall accepted D144454: Add builtin for llvm set rounding.

I have no objection to doing the documentation in a separate patch. LGTM.

Mon, Mar 6, 1:01 AM · Restricted Project, Restricted Project

Fri, Mar 3

rjmccall added a comment to D144454: Add builtin for llvm set rounding.

Okay, so if we're adding this builtin, and it's not just imitating a stdlib function, we should definitely document it as a language extension. There's a section in the manual about controlling FP modes which is probably an appropriate place for this.

Fri, Mar 3, 9:20 AM · Restricted Project, Restricted Project

Thu, Mar 2

rjmccall added a comment to D144454: Add builtin for llvm set rounding.

__builtin_set_flt_rounds seems better.

Thu, Mar 2, 1:25 PM · Restricted Project, Restricted Project

Wed, Mar 1

rjmccall added a comment to D144454: Add builtin for llvm set rounding.

I see. If we're going to take the target-independent values specified by FLT_ROUNDS, then the original builtin name is more appropriate. Of course, this has the disadvantage of not allowing target-specific values that might exist beyond those specified in the standard; are we pretty certain that's not a problem in practice?

Wed, Mar 1, 10:36 PM · Restricted Project, Restricted Project
rjmccall added a comment to D144454: Add builtin for llvm set rounding.

New builtins should be documented in the user manual.

There are standard pragmas for changing the rounding mode, right? What's the interaction between the ability to set this dynamically with a builtin call and those pragmas?

Hi, @rjmccall
Current status is LLVM has added llvm.get.rounding and llvm.set.rounding intrinsic and has also added "__builtin_flt_rounds" for llvm.get.rounding but there is no corresponding for llvm.set.rounding intrinisc. According to original patch to add llvm.set.rounding: https://reviews.llvm.org/D74729 and LLVM document for this intrinsic: https://llvm.org/docs/LangRef.html#id1506
The introduction of llvm.set.rounding intrinsic can provide same functionality as C library function "fesetround" and avoid unnecessary dependency on C library. Currently, this intrinsic can't benefit C/C++ developers since we don't have a corresponding builtin and we even can't add a C/C++ test for this intrinsic.

Wed, Mar 1, 10:23 AM · Restricted Project, Restricted Project

Feb 21 2023

rjmccall added a comment to D144454: Add builtin for llvm set rounding.

New builtins should be documented in the user manual.

Feb 21 2023, 10:36 AM · Restricted Project, Restricted Project

Feb 13 2023

rjmccall added a comment to D143919: [Clang] Copy strictfp attribute from pattern to instantiation.

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

Could you please point me out to this code? I didn't find it.

Feb 13 2023, 12:20 PM · Restricted Project, Restricted Project

Feb 3 2023

rjmccall added a comment to D142948: [OpenCL] Disable vector to scalar types coercion for OpenCL.

Does the x86 backend lower these small vectors to the same ABI? If so, I think we could just teach Clang unconditionally that it doesn't need this coercion on x86 targets. If not, this is an ABI break; are you sure that's okay?

Feb 3 2023, 1:03 PM · Restricted Project, Restricted Project
rjmccall added a comment to D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz..

Hi,

I have a question for the delete function call sinking in -Oz.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf.
According to 3.7.4.2/3
The value of the first argument supplied to a deallocation function may be a null pointer value; if so, and if the deallocation function is one supplied in the standard library, the call has no effect. Otherwise, the behavior is undefined

It seems the null checking can be skipped only when the delete is not from the standard library(have been overloaded by the user), isn't it?

The paragraph you just quoted says that the deallocation functions provided by the standard library are required have no effect on null pointers. That means it's fine to call them with a null pointer even if the source wouldn't normally execute that call. We could just insert these calls into random other code if we wanted to.

Is a null operand of delete in the source code no effect because there will be null pointer checking generated? Or the delete(null) in assembly code also valid?

Feb 3 2023, 12:57 PM · Restricted Project, Restricted Project, Restricted Project
rjmccall added a comment to D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz..

Hi,

I have a question for the delete function call sinking in -Oz.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf.
According to 3.7.4.2/3
The value of the first argument supplied to a deallocation function may be a null pointer value; if so, and if the deallocation function is one supplied in the standard library, the call has no effect. Otherwise, the behavior is undefined

It seems the null checking can be skipped only when the delete is not from the standard library(have been overloaded by the user), isn't it?

Feb 3 2023, 12:35 AM · Restricted Project, Restricted Project, Restricted Project

Feb 2 2023

rjmccall added a comment to D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null.

This seems reasonable to me. Eli, are your questions answered?

Feb 2 2023, 1:41 AM · Restricted Project, Restricted Project

Jan 26 2023

rjmccall added inline comments to D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null.
Jan 26 2023, 2:18 PM · Restricted Project, Restricted Project

Jan 25 2023

rjmccall added a comment to D129008: [Clang][OpenMP] Fix the issue that globalization doesn't work with byval struct function argument.

Right. C structs can require more than just memcpy to copy in several different situations (all involving language extensions), but it doesn't require Sema to be involved: it doesn't introduce uses of user declarations, and the compiler can figure out the work it needs to do with a straightforward recursive inspection of the field types, so IRGen just synthesizes those operations automatically when requested. Neither condition holds in C++, so Sema has to synthesize a copy expression which resolves the correct copy constructor and sets up any extra arguments it might require, triggering appropriate diagnostics and/or tracking of used decls. There are a bunch of places we do that for various language extensions already, including a bunch in the OpenMP code, and we usually just store the expression in the associated AST node.

Jan 25 2023, 8:16 PM · Restricted Project, Restricted Project
rjmccall added a comment to D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null.

I'm having a little trouble following what the meaning of an "Address" is with this patch. The Pointer member refers to an encoded value, and calling getPointer() emits IR to decode it? Could use a few comments to explain the API.

Jan 25 2023, 7:58 PM · Restricted Project, Restricted Project

Jan 23 2023

rjmccall accepted D141765: [FPEnv] Fix complex operations in strictfp mode.

Seems right to me.

Jan 23 2023, 4:40 PM · Restricted Project, Restricted Project

Jan 19 2023

rjmccall accepted D142001: [clang] Use FP options from AST for emitting code for casts.

LGTM, thanks.

Jan 19 2023, 12:57 PM · Restricted Project, Restricted Project
rjmccall added a comment to D142001: [clang] Use FP options from AST for emitting code for casts.

Overall looks reasonable. One question about testing.

Jan 19 2023, 12:19 AM · Restricted Project, Restricted Project

Jan 11 2023

rjmccall added a comment to D141381: [codegen] Store address of indirect arguments on the stack.

That's about what I would expect. One or two extra instructions per argument that are trivially removed in debug builds. Very small overall impact because there just aren't very many of these arguments.

Jan 11 2023, 2:13 PM · Restricted Project, Restricted Project

Jan 6 2023

rjmccall added a comment to D140359: [ItaniumDemangle] Fix substitution failure of _BitInt.

If it's not substituted by Clang, then Clang is actually matching the ABI, and it would need to be fixed in both places. What other implementation is substituting it?

Jan 6 2023, 10:02 AM · Restricted Project, Restricted Project, Restricted Project

Jan 5 2023

rjmccall added a comment to D140359: [ItaniumDemangle] Fix substitution failure of _BitInt.

That's an interesting question. The ABI says:

Jan 5 2023, 1:42 PM · Restricted Project, Restricted Project, Restricted Project

Jan 4 2023

rjmccall added a comment to D140868: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn.

I am not a C language lawyer :) I wonder what else should be done to move this patch forward.
The https://github.com/llvm/llvm-project/issues/59792 has got some traction and has been added a candidate for the next 15.x patch release https://github.com/llvm/llvm-project/milestone/18

Jan 4 2023, 7:32 PM · Restricted Project, Restricted Project

Jan 3 2023

rjmccall accepted D139564: clang: Don't emit "frame-pointer"="none".
Jan 3 2023, 2:26 PM · Restricted Project
rjmccall added a comment to D139564: clang: Don't emit "frame-pointer"="none".

Seems fine.

Jan 3 2023, 2:26 PM · Restricted Project
rjmccall added a comment to D140868: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn.

Isn't the C feature not technically part of the type? I thought Clang was fairly unique in modeling noreturn the way we do.

Jan 3 2023, 1:02 PM · Restricted Project, Restricted Project
rjmccall added a comment to D140868: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn.

LGTM, but I have a suggested elaboration for the comment.

Jan 3 2023, 12:41 PM · Restricted Project, Restricted Project

Dec 19 2022

rjmccall added a comment to D137343: [clang] add -Wvla-stack-allocation.

Aaron's suggested design makes the most sense to me, of all the designs I've seen here.

Dec 19 2022, 9:42 PM · Restricted Project, Restricted Project
rjmccall accepted D136639: [CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation.

Oh, I see. That's a really unfortunate way to end up emitting this code pattern, since ignoring the result is so common. To fix that, we'd have to either figure out the result was unused in Sema or do a relatively complex analysis in IRGen, though.

Dec 19 2022, 9:37 PM · Restricted Project, Restricted Project

Dec 16 2022

rjmccall added inline comments to D136639: [CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation.
Dec 16 2022, 11:05 PM · Restricted Project, Restricted Project

Dec 13 2022

rjmccall added a comment to D139163: Utils: Add utility pass to lower ifuncs.

To answer your questions in the comments about what to do about resolvers with arguments: At least glibc always calls ifunc resolvers without any arguments. It just reads the address of the resolver function from the ELF file, casts it to a pointer to a void-returning function with no arguments and calls it.

So, I agree: Resolvers with arguments should not be allowed.

@rjmccall, I'm trying to remember whether Mach-O has an ifunc-like thing that takes an argument.

I don't know that we support ifuncs of any kind, but I'll ask.

Dec 13 2022, 6:32 PM · Restricted Project, Restricted Project
rjmccall accepted D138316: [ASTContext] Avoid duplicating address space map. NFCI.

This looks fine to me.

Dec 13 2022, 11:49 AM · Restricted Project, Restricted Project
rjmccall added a comment to D139163: Utils: Add utility pass to lower ifuncs.

To answer your questions in the comments about what to do about resolvers with arguments: At least glibc always calls ifunc resolvers without any arguments. It just reads the address of the resolver function from the ELF file, casts it to a pointer to a void-returning function with no arguments and calls it.

So, I agree: Resolvers with arguments should not be allowed.

@rjmccall, I'm trying to remember whether Mach-O has an ifunc-like thing that takes an argument.

Dec 13 2022, 9:39 AM · Restricted Project, Restricted Project

Dec 5 2022

rjmccall accepted D138511: [CodeGen][AArch64] Fix AArch64ABIInfo::EmitAAPCSVAArg crash with empty record type in variadic arg.

Please don't ping every day. We haven't lost track of your patch, we're just busy reviewing other things.

Dec 5 2022, 12:28 PM · Restricted Project, Restricted Project
rjmccall added a comment to D139295: [Coroutines] Don't mark the parameter attribute of resume function as noalias.

I don't know what you mean about GEPs violating noalias. As I understand it, noalias is like restrict; it says that it can be assumed that nothing aliases the parameter except pointers derived from it. GEPs derive pointers in a way that is explicitly permitted under that.

Dec 5 2022, 10:39 AM · Restricted Project, Restricted Project, Restricted Project

Dec 2 2022

rjmccall accepted D139171: Don't revisit the subexpressions of PseudoObjectExpr when building a ParentMap.

LGTM

Dec 2 2022, 10:40 AM · Restricted Project, Restricted Project

Dec 1 2022

rjmccall accepted D138296: [clang] Avoid duplicating ProgramAddressSpace in TargetInfo. NFCI.

I think this is fine. Most of the patch is changing calls to getTargetAddressSpace to be internal to IRGen, which, as mentioned, I think is the right move.

Dec 1 2022, 10:29 AM · Restricted Project, Restricted Project

Nov 30 2022

rjmccall accepted D138295: [clang][TargetInfo] Use LangAS for getPointer{Width,Align}().

LGTM

Nov 30 2022, 10:22 AM · Restricted Project, Restricted Project

Nov 29 2022

rjmccall added a comment to D138295: [clang][TargetInfo] Use LangAS for getPointer{Width,Align}().

This looks great, thanks! Skimmed through the changes pretty quickly.

Nov 29 2022, 4:46 PM · Restricted Project, Restricted Project

Nov 28 2022

rjmccall added a comment to D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour.

The more i stare at this. The more worried i am. Is the idea that, when we are in termination/UB EH scope, we no longer have to play by the RAII rules, and destructors, for the moment, are no-ops and are side-effect free?

Nov 28 2022, 7:16 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
rjmccall accepted D138679: [clang][CodeGen] Add default attributes to __clang_call_terminate.

LGTM

Nov 28 2022, 2:03 PM · Restricted Project, Restricted Project
rjmccall added a comment to D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour.

ping. We need to get this going, reminder, this was caught because it caused a visible miscompilation.

Nov 28 2022, 2:00 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
rjmccall added a comment to D126818: Itanium ABI: Implement mangling for constrained friends.

To be clear, my opinion is just that the requested change is implementable, as I understand it.

I agree that each version of what we THINK the requested change might be is implementable, but at the moment, what the requested change IS is clear as mud :)

Nov 28 2022, 11:52 AM · Restricted Project
rjmccall added a comment to D126818: Itanium ABI: Implement mangling for constrained friends.

To be clear, my opinion is just that the requested change is implementable, as I understand it. I'm not trying to advocate for any specific language change.

Nov 28 2022, 10:21 AM · Restricted Project

Nov 18 2022

rjmccall added a comment to D138296: [clang] Avoid duplicating ProgramAddressSpace in TargetInfo. NFCI.

Functionally this looks ok to me. However I am not sure if CodeGenTypes is the 'right' place for this function to live, considering that other functions with similar functionality are in ASTContext - including overloads of getTargetAddressSpace( ). @erichkeane @aaron.ballman could you please take a look?

My view is that the parts that interact with LLVM IR should really live in CodeGen/ and not Basic/ or AST/. I will see how difficult it would be to move the remaining target (LLVM IR) address space handling code to CodeGen/

Nov 18 2022, 10:31 AM · Restricted Project, Restricted Project

Nov 17 2022

rjmccall added a comment to D126818: Itanium ABI: Implement mangling for constrained friends.

I'm too often slow to actually apply edits to the ABI document. There's been plenty of time for feedback on this one; go ahead and act like it's accepted.

Nov 17 2022, 8:49 PM · Restricted Project

Nov 16 2022

rjmccall accepted D137719: [clang] Missed rounding mode use in constant evaluation.

Looks good to me.

Nov 16 2022, 12:12 PM · Restricted Project, Restricted Project
rjmccall accepted D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg.

LGTM

Nov 16 2022, 10:57 AM · Restricted Project, Restricted Project
rjmccall added a comment to D88325: IR: Reject unsized sret in verifier.

If the type is just used for optimization purposes, we have other attributes that work more generally for pointer-typed arguments.

Nov 16 2022, 10:42 AM · Restricted Project, Restricted Project
rjmccall added a comment to D88325: IR: Reject unsized sret in verifier.

Inconsistency between things that are different is expected. byval intrinsically requires a sized type because the compiler has to know how large of an object to allocate + copy in the argument area. sret does not require a type on any target I know of, and if there are places that expect the type to be meaningful, they are almost certainly buggy in the presence of dynamically-sized types. While C has only very restricted support for dynamically-sized types, I believe Fortran's support is broader, and so psABIs usually nod at them in various ways — it's not something that's novel to Swift.

Nov 16 2022, 9:28 AM · Restricted Project, Restricted Project

Nov 15 2022

rjmccall accepted D138058: [Sema] Use the value category of the base expression when creating an ExtVectorElementExpr.

LGTM

Nov 15 2022, 4:22 PM · Restricted Project, Restricted Project
rjmccall accepted D135919: [Clang] Correct when Itanium ABI guard variables are set for non-block variables with static or thread storage duration..

Thanks, this looks great.

Nov 15 2022, 3:25 PM · Restricted Project, Restricted Project

Nov 14 2022

rjmccall added a comment to D137083: [ObjCARC] Replace parts of ObjCARCAA with intrinsic attributes.

The ARC spec describes the semantic restrictions on retain/release implementations:

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-objects-retains
Nov 14 2022, 4:18 PM · Restricted Project, Restricted Project
rjmccall updated subscribers of D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour.

I don't have a problem with adding a sanitizer like this. IIRC, though, there's a review process for adding new sanitizers; I don't remember if this is the normal RFC process or something more streamlined. Also, I know @jyknight is interested in a mode that checks -fno-exceptions more efficiently, which is of direct relevance to what you're trying here; that was discussed in a forums thread (https://discourse.llvm.org/t/rfc-add-call-unwindabort-to-llvm-ir/62543).

Nov 14 2022, 2:43 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Nov 10 2022

rjmccall added a comment to D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments.

You really can't ask whether a class template pattern is standard layout; it's not meaningful.

Well the templates have to be instantiated with concrete types as kernels can't be called from the host code. But if the concrete type is a reference or pointer there is no full instantiation apparently as AST dumps in the review description show.

Nov 10 2022, 4:06 PM · Restricted Project, Restricted Project
rjmccall added a comment to D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments.

You really can't ask whether a class template pattern is standard layout; it's not meaningful.

Nov 10 2022, 1:41 PM · Restricted Project, Restricted Project
rjmccall added a comment to D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b.

Okay. That raises the question of what the default semantics should be for std::bfloat16_t, i.e. whether we should semantically default the type to using excess-precision float arithmetic. If we did, we'd still be able to demote solitary float operations to BF16, but anything more complex than that would often force promotion without user intervention.

Nov 10 2022, 11:48 AM · Restricted Project, Restricted Project
rjmccall added a comment to D137083: [ObjCARC] Replace parts of ObjCARCAA with intrinsic attributes.

I think Akira needs to review this.

Nov 10 2022, 11:37 AM · Restricted Project, Restricted Project

Nov 4 2022

rjmccall added a comment to D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b.

On the topic of supporting BF16 arithmetic, note my comment here: https://github.com/itanium-cxx-abi/cxx-abi/pull/147#issuecomment-1254078916. To summarize, according to Steve Canon, we really shouldn't implement arithmetic directly in the BF16 format, because the precision is simply too low to be useful for intermediate results. Instead, we need to guarantee excess-precision arithmetic so that we only truncate back to BF16 when the source code requires it. We can do that in the frontend using the same excess-precision logic we added for _Float16.

Nov 4 2022, 12:47 PM · Restricted Project, Restricted Project

Nov 3 2022

rjmccall added a comment to D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b.

We talked about this on the Itanium list, and as currently specified, it is absolutely not correct for __bf16 to have the same mangling as std::bfloat16_t, because __bf16 does not have the correct semantics for std::bfloat16_t and must be a distinct type. If GCC changed __bf16 to use the new mangling without also updating the semantics, it is a bug.

That discussion was here: https://github.com/itanium-cxx-abi/cxx-abi/pull/147

If we want to implement std::bfloat16_t in Clang, we need to make it a normal arithmetic type, and in practice it needs to guarantee excess-precision arithmetic, as I discussed on that thread. Coincidentally, we did recently implement excess-precision arithmetic in Clang for _Float16.

Nov 3 2022, 1:17 PM · Restricted Project, Restricted Project
rjmccall added a comment to D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b.

We talked about this on the Itanium list, and as currently specified, it is absolutely not correct for __bf16 to have the same mangling as std::bfloat16_t, because __bf16 does not have the correct semantics for std::bfloat16_t and must be a distinct type. If GCC changed __bf16 to use the new mangling without also updating the semantics, it is a bug.

Nov 3 2022, 12:41 PM · Restricted Project, Restricted Project

Nov 2 2022

rjmccall accepted D137263: add boundary check for ASTUnresolvedSet::erase.

Good catch! Easy to see how this escaped notice for a decade, but still, it'll be good to fix.

Nov 2 2022, 4:03 PM · Restricted Project, Restricted Project

Oct 25 2022

rjmccall added inline comments to D136639: [CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation.
Oct 25 2022, 12:07 PM · Restricted Project, Restricted Project

Oct 20 2022

rjmccall accepted D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL..

LGTM

Oct 20 2022, 1:11 PM · Restricted Project, Restricted Project

Oct 18 2022

rjmccall added a comment to D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL..

Okay, this seems fine to me. I think you accidentally removed the word "promotion" from the patch title, though.

Oct 18 2022, 2:56 PM · Restricted Project, Restricted Project

Oct 17 2022

rjmccall added a comment to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.

It's certainly possible that other targets will want to support multiple C++ ABIs in the future, but it's okay for us to design things around the situation we've got today. A lot of these "ABIs" are just target-specific variants; if it simplifies code to just make ABI queries just be target queries instead of abstracting the target C++ ABI, and the resulting burden on targets with multiple supported ABIs is very small, that seems like an acceptable trade-off. If things change and we get a lot of targets asking to support multiple ABIs, there's no reason we can't revisit this decision.

Oct 17 2022, 5:27 PM · Restricted Project, Restricted Project
rjmccall added a comment to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.

Okay. So Fuchsia multilib support uses a lot of more fine-grained options rather than being arch-driven?

Oct 17 2022, 12:40 PM · Restricted Project, Restricted Project
rjmccall added a comment to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.

I suspect the Fuchsia project is not in fact volunteering to maintain a port of every imaginable C++ ABI to Fuchsia. (Many of these "ABIs" are specifically stuff like "the Itanium ABI as modified for iOS ARM64" and are not really portable anyway.) You may be interested in supporting multiple C++ ABI variants, but you've still got a list, and it's not even a very long list. So we can support this driver option, but we can lock it down to Fuchsia (or any other OSes in the future that want to support multiple ABIs), and we can lock down the options it allows.

Oct 17 2022, 12:09 PM · Restricted Project, Restricted Project
rjmccall added a comment to D136084: Fix LIT CodeGen/Func-attr.c.

With these changes in place, this looks fine to me.

Oct 17 2022, 11:20 AM · Restricted Project, Restricted Project

Oct 14 2022

rjmccall added a comment to D127695: [clang] Template Specialization Resugaring - TypeDecl.

I see, thank you. I know you've marked this a WIP, but it's always good to describe these things in the patch description; that's what it's for.

Oct 14 2022, 10:54 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
rjmccall added a comment to D127695: [clang] Template Specialization Resugaring - TypeDecl.

What is this work about?

Oct 14 2022, 6:33 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
rjmccall added a comment to D135919: [Clang] Correct when Itanium ABI guard variables are set for non-block variables with static or thread storage duration..

Please add a comment something like this:

Oct 14 2022, 4:35 PM · Restricted Project, Restricted Project
rjmccall added a comment to D135919: [Clang] Correct when Itanium ABI guard variables are set for non-block variables with static or thread storage duration..

the case from https://github.com/llvm/llvm-project/issues/57828 is not for a block-scope variable, but the case in the patch description is...

Thanks, Hubert. Yes, I found that the reported issue occurred for any case where thread safe guard variables are not required. I chose the block-scope variable example for the patch summary because I felt it better presented the issue.

Your question inspired me to do some additional testing though and I see both gcc and icc also exhibit the re-initialization behavior for that case, but not the case reported in https://github.com/llvm/llvm-project/issues/57828.

For the block-scope case, https://eel.is/c++draft/stmt.dcl#3: If control re-enters the declaration recursively while the variable is being initialized, the behavior is undefined.

Oct 14 2022, 4:09 PM · Restricted Project, Restricted Project
rjmccall added a comment to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.

If it comes down to it, we can make this a Fuchsia-specific flag, so that Fuchsia + alternative C++ ABI is essentially a sort of subtarget. That way we don't have to support the arbitrary Cartesian product of OS + ABI.

Oct 14 2022, 12:40 PM · Restricted Project, Restricted Project
rjmccall added a comment to D135919: [Clang] Correct when Itanium ABI guard variables are set for non-block variables with static or thread storage duration..

Sorry, I responded as if you were modifying the guards of thread-unsafe global variables, but you're actually modifying the guards of thread-local variables. I apologize for the confusion. However, my substantive points pretty much all still hold:

  • we have to generate code that behaves correctly in the presence of exceptions
  • providing access to an uninitialized variable is worse than recursively re-entering initialization; in either case, the program is incorrect, and the latter is much more likely to cause an immediate failure at runtime
  • the real fix requires a more elaborate code sequence to reliably generate a runtime failure
Oct 14 2022, 9:40 AM · Restricted Project, Restricted Project
rjmccall requested changes to D135919: [Clang] Correct when Itanium ABI guard variables are set for non-block variables with static or thread storage duration..

We can't set the flag if initialization is aborted by an exception, which is not ruled out by the use of thread-unsafe statics. So this is not a correct change.

Oct 14 2022, 9:30 AM · Restricted Project, Restricted Project
rjmccall added a comment to D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use.

Does Fuchsia still need this? If those experiments have stabilized, maybe we can just remove it. Also, it should probably have been a -cc1 flag in the first place.

Oct 14 2022, 9:21 AM · Restricted Project, Restricted Project

Oct 13 2022

rjmccall accepted D131701: [CodeGen][ObjC] Call synthesized copy constructor/assignment operator functions in getter/setter functions of non-trivial C struct properties.

LGTM

Oct 13 2022, 9:28 AM · Restricted Project, Restricted Project

Oct 12 2022

rjmccall added a comment to D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo.

I don't remember the history of the -fc++-abi flag at all, so if that's a driver flag, you'll probably need to investigate it more to remove it. Maybe it was added for testing purposes and only made a driver flag accidentally.

Oct 12 2022, 4:06 PM · Restricted Project, Restricted Project
rjmccall added a comment to D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo.

(abandoned this, but still kind of curious)

@rjmccall - any background/history of having the CXXABI distinct from the OS? I guess the point might've been that the C ABI is part of/the definition of the OS, but maybe the CXX ABI is more "floating"/flexible on top of that? Though it means these CXX ABIs don't get the benefit of being grouped with the rest of the targetOS - instead they're a bunch of switches, which seems a bit unfortunate, but I guess there's not lots of properties here, so maybe that's OK.

I guess the other question: Is this flexibility valuable/worthwhile, or could we fold the CXXABI back into the TargetOS?

Oct 12 2022, 11:06 AM · Restricted Project, Restricted Project
rjmccall accepted D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register.
Oct 12 2022, 10:56 AM · Restricted Project, Restricted Project

Oct 11 2022

rjmccall added a comment to D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template.

I think that's fair. I don't know how much concrete information we'll get about this in a short period of time, but it's worth trying to collect it.

Oct 11 2022, 8:18 PM · Restricted Project, Restricted Project
rjmccall added a comment to D135550: [Coroutines] Don't merging readnone calls in presplit coroutines.

Sounds fine to me.

Oct 11 2022, 8:09 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register.
Oct 11 2022, 8:08 PM · Restricted Project, Restricted Project
rjmccall added a reviewer for D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register: t.p.northover.
Oct 11 2022, 7:48 PM · Restricted Project, Restricted Project
rjmccall added a comment to D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register.

This seems be to a genuine target difference, right? PPC64 has this ABI rule:

Oct 11 2022, 10:34 AM · Restricted Project, Restricted Project
rjmccall added a comment to D135550: [Coroutines] Don't merging readnone calls in presplit coroutines.

I haven't been following all the threads about the right way to handle this. If this is the consensus for the correct way to handle this in the short term, it seems like a conceptually acceptable fix to me.

Oct 11 2022, 10:02 AM · Restricted Project, Restricted Project

Oct 10 2022

rjmccall accepted D135628: [clang][codegen] Don't emit atomic loads for threadsafe init if they aren't inline.

Makes sense. Nice code-size optimization at any rate.

Oct 10 2022, 6:04 PM · Restricted Project, Restricted Project
rjmccall accepted D131698: [Sema] Fix `ExtVectorElementExpr` tree transform for the `isArrow` case..

LGTM

Oct 10 2022, 5:55 PM · Restricted Project, Restricted Project
rjmccall added a comment to D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL..

If you have char, would you want it to promote? Because turning char to _BitInt(8) is breaking with C on other grounds (like aliasing), for better or worse. So if you just don't want promotion, maybe you really should just disable promotion.

Oct 10 2022, 2:58 PM · Restricted Project, Restricted Project
rjmccall added a comment to D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL..

Sure, but it's extremely easy to unpromote that arithmetic for most operators, and I'm sure LLVM already has a pass that will do that.

Okay... but HLSL explicitly doesn't promote. Having the compiler rely on an optimization to generate correct code is undesirable. Especially since we want debug builds to behave correctly.

Alternatively, if you're worried about your ability to unpromote, and since you're breaking strict conformance anyway, have you considered just removing the initial promotion to int from the usual arithmetic conversions? I'm pretty sure the rest of the rules hang together just fine if you do. Then you have a uniform rule that permits easier vectorization for all small integer types rather than being sensitive specifically to using the int16_t typedef.

HLSL isn't conformant to C or C++. One of the big areas that things get wonky is that every int is actually an implicit SIMD vector of a hardware-determined size.

Oct 10 2022, 2:08 PM · Restricted Project, Restricted Project
rjmccall added a comment to D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL..

Sure, but it's extremely easy to unpromote that arithmetic for most operators, and I'm sure LLVM already has a pass that will do that. The only thing that blocks unpromotion is when you feed a promoted result into something that's sensitive to working on a wider value, like comparison or division, or of course an assignment to a wider type.

Oct 10 2022, 9:37 AM · Restricted Project, Restricted Project