Re: [PATCH 3/5] Rewrite part of and_comparisons_1 into match.pd.

["Martin Liška" <mliska@suse.cz>]

On 9/11/19 2:43 PM, Richard Biener wrote:
> On Wed, 11 Sep 2019, Martin Liška wrote:
> 
>> I'm sending updated version of the patch where I changed
>> from previous version:
>> - more compact matching is used (note @3 and @4):
>>   (and (code1:c@3 @0 INTEGER_CST@1) (code2:c@4 @0 INTEGER_CST@2))
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed?
> 
> --- a/gcc/genmatch.c
> +++ b/gcc/genmatch.c
> @@ -1894,9 +1894,11 @@ dt_node *
>  dt_node::append_simplify (simplify *s, unsigned pattern_no,
>                           dt_operand **indexes)
>  {
> +  dt_simplify *s2;
>    dt_simplify *n = new dt_simplify (s, pattern_no, indexes);
>    for (unsigned i = 0; i < kids.length (); ++i)
> -    if (dt_simplify *s2 = dyn_cast <dt_simplify *> (kids[i]))
> +    if ((s2 = dyn_cast <dt_simplify *> (kids[i]))
> +       && s->match->location != s2->s->match->location)
>        {
> 
> can you retain the warning for verbose >= 1 please?  And put in
> a comment that duplicates are sometimes hard to avoid with
> nested for so this keeps match.pd sources small.

Sure.

> 
> +  /* Function maybe_fold_comparisons_from_match_pd creates temporary
> +     SSA_NAMEs.  */
> +  if (TREE_CODE (op1) == SSA_NAME && TREE_CODE (op2) == SSA_NAME)
> +    {
> +      gimple *s = SSA_NAME_DEF_STMT (op2);
> +      if (is_gimple_assign (s))
> +       return same_bool_comparison_p (op1, gimple_assign_rhs_code (s),
> +                                      gimple_assign_rhs1 (s),
> +                                      gimple_assign_rhs2 (s));
> +      else
> +       return false;
> +    }
> 
> when do you actually run into the need to add this?  The whole
> same_bool_result_p/same_bool_comparison_p helpers look a bit
> odd to me.  It shouldn't be special to the new temporaries
> either so at least the comment looks out-of place.

At some point it was needed to handle gcc/testsuite/gcc.dg/pr46909.c
test-case. Apparently, now the test-case is fine with the hunk. I will it
removal of the hunk.

> 
> +      else if (op.code.is_tree_code ()
> +              && TREE_CODE_CLASS ((tree_code)op.code) == tcc_comparison)
> +       {
> 
> COMPARISON_CLASS_P ((tree_code)op.code)
> 
> as was noted.

Won't work here:

/home/marxin/Programming/gcc/gcc/gimple-fold.c: In function ‘tree_node* maybe_fold_comparisons_from_match_pd(tree, tree_code, tree_code, tree, tree, tree_code, tree, tree)’:
/home/marxin/Programming/gcc/gcc/tree.h:239:49: error: base operand of ‘->’ is not a pointer
  239 | #define TREE_CODE(NODE) ((enum tree_code) (NODE)->base.code)
      |                                                 ^~

> 
> Any particular reason you needed to swap the calls in
> maybe_fold_and/or_comparisons?
> 
> +(for code1 (eq ne)
> + (for code2 (eq ne lt gt le ge)
> +  (for and (truth_and bit_and)
> +   (simplify
> 
> You could save some code-bloat with writing
> 
>   (for and (
> #if GENERIC
>   truth_and
> #endif
>   bit_and)
> 
> but since you are moving the patterns from GIMPLE code I'd say
> simply remove truth_and and that innermost for?

I'm gonna drop the generic tree codes support.

Martin

> 
> Otherwise OK.
> 
> Thanks,
> Richard.
> 
> 
> 
> 
>> Thanks,
>> Martin
>>
>