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

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

On Wed, 7 Feb 2024 at 16:25, Torbjorn SVENSSON <
torbjorn.svensson@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, <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.
>
>