In D146148#4217382, @zahiraam wrote:

In D146148#4213475, @rjmccall wrote:

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

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.

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.

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?

In D83906#4182287, @dexonsmith wrote:

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

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.

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

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.

__builtin_set_flt_rounds seems better.

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?

In D144454#4157717, @xiongji90 wrote:

In D144454#4142253, @rjmccall wrote:

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.

New builtins should be documented in the user manual.

In D143919#4123712, @sepavloff wrote:

In D143919#4123616, @efriedma wrote:

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.

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?

In D79378#4101935, @shiva0217 wrote:

In D79378#4101829, @rjmccall wrote:

In D79378#4101613, @shiva0217 wrote:

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?

In D79378#4101613, @shiva0217 wrote:

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?

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

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.

In D142584#4081345, @efriedma wrote:

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.

Seems right to me.

LGTM, thanks.

Overall looks reasonable. One question about testing.

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.

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?

That's an interesting question. The ABI says:

In D140868#4027480, @MaskRay wrote:

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

Seems fine.

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

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

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

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.

In D139163#3992391, @rjmccall wrote:

In D139163#3992262, @dexonsmith wrote:

In D139163#3991248, @MoritzS wrote:

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.

This looks fine to me.

In D139163#3992262, @dexonsmith wrote:

In D139163#3991248, @MoritzS wrote:

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.

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

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.

LGTM

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.

LGTM

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

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?

LGTM

In D137381#3953178, @lebedev.ri wrote:

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

In D126818#3954139, @erichkeane wrote:

In D126818#3954137, @rjmccall wrote:

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

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.

In D138296#3937486, @arichardson wrote:

In D138296#3937224, @eandrews wrote:

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/

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.

Looks good to me.

LGTM

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

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.

LGTM

Thanks, this looks great.

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

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#arc-objects-retains

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

In D134445#3920257, @Anastasia wrote:

In D134445#3920188, @rjmccall wrote:

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.

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

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.

I think Akira needs to review this.

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.

In D136919#3906133, @rjmccall wrote:

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.

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.

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

LGTM

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

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.

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

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.

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

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.

What is this work about?

Please add a comment something like this:

In D135919#3859520, @hubert.reinterpretcast wrote:

In D135919#3859491, @tahonermann wrote:

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.

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.

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

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.

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.

LGTM

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.

In D135326#3851678, @dblaikie wrote:

In D135326#3851672, @dblaikie wrote:

(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?

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.

Sounds fine to me.

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

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.

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

LGTM

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.

In D133668#3847807, @beanz wrote:

In D133668#3847163, @rjmccall wrote:

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.

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.