| Message ID | 001a8c05-7674-6078-d4ec-087ecc109464@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Transform assertion into optimization hints | expand |
On Mon, 17 Sep 2018, François Dumont wrote: > We talk about it a while back. > > I've run testsuite several times since I have this patch on my local copy. > Note that when I implemented it the wrong way tests started to fail so it is > clearly having an effect on the generated code. > > * include/bits/c++config [__OPTIMIZE__](__glibcxx_assert): Define as > optimization hint > > using __builtin_unreachable. > > Ok to commit ? I see for instance in bits/regex_automaton.tcc: __glibcxx_assert(__m.count(__ref._M_next) > 0); where __m is a map, which does not look so well suited for a __builtin_unreachable. Is it using the wrong macro?
On 09/17/2018 11:15 PM, Marc Glisse wrote: > On Mon, 17 Sep 2018, François Dumont wrote: > >> We talk about it a while back. >> >> I've run testsuite several times since I have this patch on my local >> copy. Note that when I implemented it the wrong way tests started to >> fail so it is clearly having an effect on the generated code. >> >> * include/bits/c++config [__OPTIMIZE__](__glibcxx_assert): Define as >> optimization hint >> >> using __builtin_unreachable. >> >> Ok to commit ? > > I see for instance in bits/regex_automaton.tcc: > > __glibcxx_assert(__m.count(__ref._M_next) > 0); > > where __m is a map, which does not look so well suited for a > __builtin_unreachable. Is it using the wrong macro? > I don't know if this particular code is well covered by the testsuite. So I tweaked several tests in 23_containers/map and the code compiles just fine if it is your concern. If your concern is rather that the condition got evaluated which will eventually slow down execution then I need to check generated code. Any good link explaining how to have a clear view on the generated code ? Maybe a 'if __builtin_constant_p(_Condition)' could help if gcc doesn't do it itself already.
On Tue, 18 Sep 2018, François Dumont wrote: > If your concern is rather that the condition got evaluated which will > eventually slow down execution then I need to check generated code. Any good > link explaining how to have a clear view on the generated code ? This. For a file like #include <map> void f(); void g(std::map<int,int> const&m){ if(m.count(42)>0)__builtin_unreachable(); f(); } compiled with g++ file.c -O3 -S -fdump-tree-optimized, I get a file.c.228t.optimized that contains quite a lot of code, not just a call to f. > Maybe a 'if __builtin_constant_p(_Condition)' could help if gcc doesn't do it > itself already. How would you use that precisely? It may be easiest to use a different macro for trivial tests that can go with __builtin_unreachable and for expensive tests that cannot.
On 09/18/2018 08:11 AM, Marc Glisse wrote: > On Tue, 18 Sep 2018, François Dumont wrote: > >> If your concern is rather that the condition got evaluated which will >> eventually slow down execution then I need to check generated code. >> Any good link explaining how to have a clear view on the generated >> code ? > > This. > > For a file like > > #include <map> > > void f(); > void g(std::map<int,int> const&m){ > if(m.count(42)>0)__builtin_unreachable(); > f(); > } > > compiled with g++ file.c -O3 -S -fdump-tree-optimized, I get a > file.c.228t.optimized that contains quite a lot of code, not just a call > to f. Too bad, and thanks for the tip on how to generate this. > >> Maybe a 'if __builtin_constant_p(_Condition)' could help if gcc >> doesn't do it itself already. > > How would you use that precisely? I wouldn't, I wasn't totally waken up when I proposed this, it makes no sens. > > It may be easiest to use a different macro for trivial tests that can go > with __builtin_unreachable and for expensive tests that cannot. > Even if I think that doing this kind of operation in an assert call is too much I agree that it makes this change invalid. I'll see if I need to introduce a macro the day I want to add a __builtin_unreachable. François
On 17/09/18 23:15 +0200, Marc Glisse wrote: >On Mon, 17 Sep 2018, François Dumont wrote: > >>We talk about it a while back. >> >>I've run testsuite several times since I have this patch on my local >>copy. Note that when I implemented it the wrong way tests started to >>fail so it is clearly having an effect on the generated code. >> >>* include/bits/c++config [__OPTIMIZE__](__glibcxx_assert): Define as >>optimization hint >> >>using __builtin_unreachable. >> >>Ok to commit ? > >I see for instance in bits/regex_automaton.tcc: > > __glibcxx_assert(__m.count(__ref._M_next) > 0); > >where __m is a map, which does not look so well suited for a >__builtin_unreachable. Is it using the wrong macro? Yes, that looks like it's checking the implementation, but we should only add assertions to check what users do (we just need to get our own implementation right). PR 85184 points out some similar assertions.
On 19/09/18 10:16 +0100, Jonathan Wakely wrote: >On 17/09/18 23:15 +0200, Marc Glisse wrote: >>On Mon, 17 Sep 2018, François Dumont wrote: >> >>>We talk about it a while back. >>> >>>I've run testsuite several times since I have this patch on my >>>local copy. Note that when I implemented it the wrong way tests >>>started to fail so it is clearly having an effect on the generated >>>code. >>> >>>* include/bits/c++config [__OPTIMIZE__](__glibcxx_assert): Define >>>as optimization hint >>> >>>using __builtin_unreachable. >>> >>>Ok to commit ? >> >>I see for instance in bits/regex_automaton.tcc: >> >> __glibcxx_assert(__m.count(__ref._M_next) > 0); >> >>where __m is a map, which does not look so well suited for a >>__builtin_unreachable. Is it using the wrong macro? > >Yes, that looks like it's checking the implementation, but we should >only add assertions to check what users do (we just need to get our >own implementation right). I'll test this patch. diff --git a/libstdc++-v3/include/bits/regex_automaton.tcc b/libstdc++-v3/include/bits/regex_automaton.tcc index 7a0e6a36a7a..5993fcfeeaf 100644 --- a/libstdc++-v3/include/bits/regex_automaton.tcc +++ b/libstdc++-v3/include/bits/regex_automaton.tcc @@ -220,16 +220,9 @@ namespace __detail auto __v = __it.second; auto& __ref = _M_nfa[__v]; if (__ref._M_next != _S_invalid_state_id) - { - __glibcxx_assert(__m.count(__ref._M_next) > 0); - __ref._M_next = __m[__ref._M_next]; - } - if (__ref._M_has_alt()) - if (__ref._M_alt != _S_invalid_state_id) - { - __glibcxx_assert(__m.count(__ref._M_alt) > 0); - __ref._M_alt = __m[__ref._M_alt]; - } + __ref._M_next = __m.find(__ref._M_next)->second; + if (__ref._M_has_alt() && __ref._M_alt != _S_invalid_state_id) + __ref._M_alt = __m.find(__ref._M_alt)->second; } return _StateSeq(_M_nfa, __m[_M_start], __m[_M_end]); }
On 19/09/18 10:42 +0100, Jonathan Wakely wrote: >On 19/09/18 10:16 +0100, Jonathan Wakely wrote: >>On 17/09/18 23:15 +0200, Marc Glisse wrote: >>>On Mon, 17 Sep 2018, François Dumont wrote: >>> >>>>We talk about it a while back. >>>> >>>>I've run testsuite several times since I have this patch on my >>>>local copy. Note that when I implemented it the wrong way tests >>>>started to fail so it is clearly having an effect on the >>>>generated code. >>>> >>>>* include/bits/c++config [__OPTIMIZE__](__glibcxx_assert): >>>>Define as optimization hint >>>> >>>>using __builtin_unreachable. >>>> >>>>Ok to commit ? >>> >>>I see for instance in bits/regex_automaton.tcc: >>> >>> __glibcxx_assert(__m.count(__ref._M_next) > 0); >>> >>>where __m is a map, which does not look so well suited for a >>>__builtin_unreachable. Is it using the wrong macro? >> >>Yes, that looks like it's checking the implementation, but we should >>only add assertions to check what users do (we just need to get our >>own implementation right). > >I'll test this patch. Committed to trunk. * include/bits/regex_automaton.tcc (_StateSeq<_TraitsT>::_M_clone()): Remove __glibcxx_assert statements and use map::find instead of map::operator[]. >diff --git a/libstdc++-v3/include/bits/regex_automaton.tcc b/libstdc++-v3/include/bits/regex_automaton.tcc >index 7a0e6a36a7a..5993fcfeeaf 100644 >--- a/libstdc++-v3/include/bits/regex_automaton.tcc >+++ b/libstdc++-v3/include/bits/regex_automaton.tcc >@@ -220,16 +220,9 @@ namespace __detail > auto __v = __it.second; > auto& __ref = _M_nfa[__v]; > if (__ref._M_next != _S_invalid_state_id) >- { >- __glibcxx_assert(__m.count(__ref._M_next) > 0); >- __ref._M_next = __m[__ref._M_next]; >- } >- if (__ref._M_has_alt()) >- if (__ref._M_alt != _S_invalid_state_id) >- { >- __glibcxx_assert(__m.count(__ref._M_alt) > 0); >- __ref._M_alt = __m[__ref._M_alt]; >- } >+ __ref._M_next = __m.find(__ref._M_next)->second; >+ if (__ref._M_has_alt() && __ref._M_alt != _S_invalid_state_id) >+ __ref._M_alt = __m.find(__ref._M_alt)->second; > } > return _StateSeq(_M_nfa, __m[_M_start], __m[_M_end]); > }
diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config index d499d32b51e..24bdc97e0f5 100644 --- a/libstdc++-v3/include/bits/c++config +++ b/libstdc++-v3/include/bits/c++config @@ -466,6 +466,9 @@ namespace std #if defined(_GLIBCXX_ASSERTIONS) # define __glibcxx_assert(_Condition) __glibcxx_assert_impl(_Condition) +#elif defined(__OPTIMIZE__) +# define __glibcxx_assert(_Condition) \ + do { if (! (_Condition)) __builtin_unreachable(); } while (false) #else # define __glibcxx_assert(_Condition) #endif