On Wed, 7 Feb 2024 at 16:25, Torbjorn SVENSSON < torbjorn.svens...@foss.st.com> wrote:
> Hi, > > Is it okay to backport e39b3e02c27bd771a07e385f9672ecf1a45ced77 to > releases/gcc-13? > It would also need 807f47497f17ed50be91f0f879308cb6fa063966 Please test with that as well, and OK for both if all goes well. > Without this backport, I see this failure on arm-none-eabi: > > FAIL: 23_containers/vector/bool/110807.cc (test for excess errors) > > Kind regards, > Torbjörn > > > On 2023-11-09 02:22, Jonathan Wakely wrote: > > > > > > On Thu, 9 Nov 2023, 01:17 Alexandre Oliva, <ol...@adacore.com > > <mailto:ol...@adacore.com>> wrote: > > > > On Nov 8, 2023, Jonathan Wakely <jwak...@redhat.com > > <mailto:jwak...@redhat.com>> wrote: > > > > > A single underscore prefix on __GLIBCXX_BUILTIN_ASSUME and > > > __GLIBCXX_DISABLE_ASSUMPTIONS please. > > > > That's entirely gone now. > > > > >> + do \ > > >> + if (std::is_constant_evaluated ()) \ > > >> + static_assert(expr); \ > > > > > This can never be valid. > > > > *nod* > > > > > This already works fine in constant evaluation anyway. > > > > Yeah, that's what I figured. > > > > > But what's the null dereference for? > > > > The idea was to clearly trigger undefined behavior. Maybe it wasn't > > needed, it didn't occur to me that __builtin_unreachable() would be > > enough. I realize I was really trying to emulate attribute assume, > even > > without knowing it existed ;-) > > > > >> +#define __GLIBCXX_BUILTIN_ASSUME(expr) \ > > >> + (void)(false && (expr)) > > > > > What's the point of this, just to verify that (expr) is > contextually > > > convertible to bool? > > > > I'd have phrased it as "avoid the case in which something compiles > with > > -O0 but not with -O", but yeah ;-) > > > > > We don't use the _p suffix for predicates in the library. > > > Please use just _M_normalized or _M_is_normalized. > > > > ACK. It's also gone now. > > > > > But do we even need this function? It's not used anywhere else, > > can we > > > just inline the condition into _M_assume_normalized() ? > > > > I had other uses for it in earlier versions of the patch, but it > makes > > no sense any more indeed. > > > > >> + _GLIBCXX20_CONSTEXPR > > >> + void > > >> + _M_assume_normalized() const > > > > > I think this should use _GLIBCXX_ALWAYS_INLINE > > > > *nod*, thanks > > > > >> + { > > >> + __GLIBCXX_BUILTIN_ASSUME (_M_normalized_p ()); > > > > > Is there even any benefit to this macro? > > > > I just thought it could have other uses, without being aware that the > > entire concept was available as a statement attribute. Funny, I'd > even > > searched for it among the existing attributes and builtins, but > somehow > > I managed to miss it. Thanks for getting me back down that path. > > > > > __attribute__((__assume__(_M_offset < > > unsigned(_S_word_bit)))); > > > > That unfortunately doesn't work, because the assume lowering doesn't > go > > as far as dereferencing the implicit this and making an SSA_NAME out > of > > the loaded _M_offset, which we'd need to be able to optimize based on > > it. But that only took me a while to figure out and massage into > > something that had the desired effect. Now, maybe the above *should* > > have that effect already, but unfortunately it doesn't. > > > > > Maybe even get rid of _M_assume_normalized() as a function and > just > > > put that attribute everywhere you currently use > _M_assume_normalized. > > > > Because of the slight kludge required to make the attribute have the > > desired effect (namely ensuring the _M_offset reference is > evaluated), > > I've retained it as an inline function. > > > > Here's what I'm retesting now. WDYT? > > > > > > ofst needs to be __ofst but OK for trunk with that change. > > > > We probably want this on the gcc-13 branch too, but let's give it some > > time on trunk in case the assume attribute isn't quite ready for prime > time. > >
