Transform assertion into optimization hints

Transform assertion into optimization hints

[François Dumont]

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

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 ?

François


[-- Attachment #2: optimization.patch --]
[-- Type: text/x-patch, Size: 550 bytes --]

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

Re: Transform assertion into optimization hints

[Marc Glisse]

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?

-- 
Marc Glisse

Re: Transform assertion into optimization hints

[François Dumont]

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.

Re: Transform assertion into optimization hints

[Marc Glisse]

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.

-- 
Marc Glisse

Re: Transform assertion into optimization hints

[François Dumont]

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

Re: Transform assertion into optimization hints

[Jonathan Wakely]

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.

Re: Transform assertion into optimization hints

[Jonathan Wakely]

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

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.



[-- Attachment #2: patch.txt --]
[-- Type: text/x-patch, Size: 939 bytes --]

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]);
     }

Re: Transform assertion into optimization hints

[Jonathan Wakely]

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]);
>     }