User Details
- User Since
- Jan 18 2013, 11:30 AM (530 w, 6 d)
Today
Yesterday
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.
Tue, Mar 21
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.
Mon, Mar 20
Sun, Mar 19
Thu, Mar 16
Thu, Mar 9
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?
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.
Mon, Mar 6
I have no objection to doing the documentation in a separate patch. LGTM.
Fri, Mar 3
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.
Thu, Mar 2
__builtin_set_flt_rounds seems better.
Wed, Mar 1
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?
Feb 21 2023
New builtins should be documented in the user manual.
Feb 13 2023
Feb 3 2023
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 2 2023
This seems reasonable to me. Eli, are your questions answered?
Jan 26 2023
Jan 25 2023
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 23 2023
Seems right to me.
Jan 19 2023
LGTM, thanks.
Overall looks reasonable. One question about testing.
Jan 11 2023
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 6 2023
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 5 2023
That's an interesting question. The ABI says:
Jan 4 2023
Jan 3 2023
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.
Dec 19 2022
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.
Dec 16 2022
Dec 13 2022
This looks fine to me.
Dec 5 2022
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.
Dec 2 2022
LGTM
Dec 1 2022
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.
Nov 30 2022
LGTM
Nov 29 2022
This looks great, thanks! Skimmed through the changes pretty quickly.
Nov 28 2022
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
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 18 2022
Nov 17 2022
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 16 2022
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.
Nov 15 2022
LGTM
Thanks, this looks great.
Nov 14 2022
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).
Nov 10 2022
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.
Nov 4 2022
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 3 2022
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 2 2022
Good catch! Easy to see how this escaped notice for a decade, but still, it'll be good to fix.
Oct 25 2022
Oct 20 2022
LGTM
Oct 18 2022
Okay, this seems fine to me. I think you accidentally removed the word "promotion" from the patch title, though.
Oct 17 2022
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.
Oct 14 2022
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:
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.
Oct 13 2022
LGTM
Oct 12 2022
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 11 2022
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.
Oct 10 2022
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.
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.