Re: [PATCH v2] libstdc++: optimize bit iterators assuming normalization [PR110807]

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

Hi,

Is it okay to backport e39b3e02c27bd771a07e385f9672ecf1a45ced77 to 
releases/gcc-13?

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, <oliva@adacore.com 
> <mailto:oliva@adacore.com>> wrote:
> 
>     On Nov  8, 2023, Jonathan Wakely <jwakely@redhat.com
>     <mailto:jwakely@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.