[PATCHv2] Add a couple of A?CST1:CST2 match and simplify optimizations

[Andrew MacLeod]

On 5/26/21 1:07 PM, Bernd Edlinger wrote:
> On 5/26/21 7:03 PM, Bernd Edlinger wrote:
>> On 5/26/21 2:05 PM, Richard Biener wrote:
>>> On Wed, May 26, 2021 at 1:37 PM Andrew Pinski <pinskia@gmail.com> wrote:
>>>> On Wed, May 26, 2021 at 4:28 AM Richard Biener
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, May 26, 2021 at 1:07 PM Andrew Pinski <pinskia@gmail.com> wrote:
>>>>>> On Wed, May 26, 2021 at 2:01 AM Andrew Pinski <pinskia@gmail.com> wrote:
>>>>>>> On Wed, May 26, 2021 at 1:43 AM Bernd Edlinger
>>>>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>>>>> On 5/25/21 4:22 PM, Richard Biener via Gcc-patches wrote:
>>>>>>>>> On Sun, May 23, 2021 at 12:03 PM apinski--- via Gcc-patches
>>>>>>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>>>>>> From: Andrew Pinski <apinski@marvell.com>
>>>>>>>>>>
>>>>>>>>>> Instead of some of the more manual optimizations inside phi-opt,
>>>>>>>>>> it would be good idea to do a lot of the heavy lifting inside match
>>>>>>>>>> and simplify instead. In the process, this moves the three simple
>>>>>>>>>> A?CST1:CST2 (where CST1 or CST2 is zero) simplifications.
>>>>>>>>>>
>>>>>>>>>> OK? Boostrapped and tested on x86_64-linux-gnu with no regressions.
>>>>>>>>>>
>>>>>>>>>> Differences from V1:
>>>>>>>>>> * Use bit_xor 1 instead of bit_not to fix the problem with boolean types
>>>>>>>>>> which are not 1 bit precision.
>>>>>>>>> OK.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Richard.
>>>>>>>>>
>>>>>>>> Hmm, sorry, no luck.
>>>>>>>>
>>>>>>>> I think this caused:
>>>>>>> If anything it is a bad interaction with changes between r12-1046 and
>>>>>>> r12-1053; I am suspecting a bug in those changes rather than my
>>>>>>> changes causing the bug.  Debugging it right now.
>>>>>> (gdb) p debug_tree(name)
>>>>>>   <ssa_name 0x7ffff6a5cd38
>>>>>>      type <boolean_type 0x7ffff6b45b28 _Bool public unsigned QI
>>>>>>          size <integer_cst 0x7ffff6b2bdc8 constant 8>
>>>>>>          unit-size <integer_cst 0x7ffff6b2bde0 constant 1>
>>>>>>          align:8 warn_if_not_align:0 symtab:0 alias-set -1
>>>>>> canonical-type 0x7ffff6b45b28 precision:1 min <integer_cst
>>>>>> 0x7ffff6b4a030 0> max <integer_cst 0x7ffff6b4a060 1>>
>>>>>>
>>>>>>      def_stmt _19 = ~_8;
>>>>>>      version:19>
>>>>>>
>>>>>> So what is happening is evrp converted:
>>>>>> ct_12 = ct_5 + -1;
>>>>>> Into
>>>>>> ct_12 = ct_5 == 1 ? 0 : 1;
>>>>>> (this was done before my patch)
>>>>> Note this COND_EXPR is supposed to be combined
>>>>> with its single use in a GIMPLE_COND ...
>>>> I Noticed it was not doing it (before my patch) inside evrp either.
>>> I think it is at most done in forwprop, but even then it likely
>>> lacks a fold pattern - we only seem to forward comparisons
>>> into GIMPLE_CONDs explicitely, leaving the rest to
>>> match.pd patterns.
>>>
>> How about this for a quick fix:
>>
>> commit b71621f51bc2819bb7d202efabc17fec5cc92f8f
>> Author: Bernd Edlinger <bernd.edlinger@hotmail.de>
>> Date:   Wed May 26 18:45:09 2021 +0200
>>
>>      Fix gcc-bootstrap issue
>>      
>>      ... or at least try to.
>>      
>>      2021-05-26  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>      
>>          * gimple-range-gori.cc (range_def_chain::register_dependency):
>>          Resize m_def_chain when needed.
>>
>> diff --git a/gcc/gimple-range-gori.cc b/gcc/gimple-range-gori.cc
>> index a4c4bf5..722bf5d 100644
>> --- a/gcc/gimple-range-gori.cc
>> +++ b/gcc/gimple-range-gori.cc
>> @@ -177,6 +177,8 @@ range_def_chain::register_dependency (tree name, tree dep, basic_block bb)
>>   
>>     unsigned v = SSA_NAME_VERSION (name);
>>     struct rdc &src = m_def_chain[v];
>> +  if (v >= m_def_chain.length ())
>> +    m_def_chain.safe_grow_cleared (num_ssa_names + 1);>    gimple *def_stmt = SSA_NAME_DEF_STMT (dep);
>>     unsigned dep_v = SSA_NAME_VERSION (dep);
>>     bitmap b;
>>
>>
>> Should I push this?
>> Or has anyone a better idea?
>>
> Aehm, I meant of course:
>
> --- a/gcc/gimple-range-gori.cc
> +++ b/gcc/gimple-range-gori.cc
> @@ -176,6 +176,8 @@ range_def_chain::register_dependency (tree name, tree dep, b
>       return;
>   
>     unsigned v = SSA_NAME_VERSION (name);
> +  if (v >= m_def_chain.length ())
> +    m_def_chain.safe_grow_cleared (num_ssa_names + 1);
>     struct rdc &src = m_def_chain[v];
>     gimple *def_stmt = SSA_NAME_DEF_STMT (dep);
>     unsigned dep_v = SSA_NAME_VERSION (dep);
>
>
yeah, I was just about to say this :-)    when everything was 
restructured, it seems there is a path to the new register_dependency 
call from the temporal cache which does not do the  range check...

all the other calls are gated by a call to has_def_chain() which ensures 
the vector is big enough.  This will be redundant in many cases, but 
will do for now until I do a perf test and see whether I should so 
something slightly different.

push is OK.

Andrew