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