Re: [PATCH, match] Fix pr68714

[Marc Glisse <marc dot glisse at inria dot fr>]

On Wed, 2 Mar 2016, Richard Biener wrote:

> On Tue, 1 Mar 2016, Richard Henderson wrote:
>
> > This is similar to Mark Gilsse's patch in the OP, except that it ensures that
> > the expression will fold back to a single condition.  I did include Richi's
> > patch from #c6 to make it more likely to trigger asap.
> >
> > I'd appreciate feedback on the match.pd changes; it's my first time looking
> > into this new(ish) mini-language.
>
> The recalculate_side_effects call in the gimplify.c hunk is indented
> oddly (spaces, no tabs).
>
> Note that nothing guarantees we've canoncalized the cond-exprs
> to have all-ones in the true path and zero in the false path
> the pattern will miss three of four cases ... also I don't see
> why the pattern would need to be restricted to those - shouldn't it
> simply work for
>
> +  (bcode (vec_cond COMPARISON_CLASS_P@0 @2 @3)
> +        (vec_cond COMPARISON_CLASS_P@1 @2 @3))
>
> ?  That would catch two of four cases ;)


I don't think that's always valid, you would need that @2 | @3 is the same 
as @2 for the simplification to work for bit_ior (we could use 
operand_equal_p and fold_binary to ask for that, not sure if there is a 
cleaner way).


> +           (if (COMPARISON_CLASS_P (folded))
> +             { build3 (VEC_COND_EXPR, TREE_TYPE (@2), folded, @2, @3);
> })))
>
> this is wrong - you may not build GENERIC trees here.  Instead you
> want
>
>  (if (COMPARISON_CLASS_P (folded))
>   (vec_cond { folded; } @2 @3))))
>
> though even that might be bogus if the operands of the comparison
> in 'folded' are not is_gimple_val.
>
> Now, building GENERIC via combine_comparisons is somewhat gross
> in match.pd, simple helpers that can combine two comparison
> codes would be nicer though the match.pd language doesn't allow
> easy use of that.  For example handling lt/eq and lt/le combinations
> requires sth like
>
> (for cnd (cond vec_cond)
> (for cmp1 (lt eq)
>      cmp2 (lt lt eq eq)
>      cmp  (lt le le eq)
>   (simplify
>    (bit_ior (cnd (cmp1 @0 @1) @2 @3)
>             (cnd (cmp2 @0 @1) @2 @3))
>    (cnd (cmp @0 @1) @2 @3)))
> (for cmp1 (lt le)
>      cmp2 (lt lt le le)
>      cmp  (lt lt lt le)
>   (simplify
>    (bit_and (cnd (cmp1 @0 @1) @2 @3)
>             (cnd (cmp2 @0 @1) @2 @3))
>    (cnd (cmp @0 @1) @2 @3))))
>
> and it will also code-generate the comparison outside of the
> [VEC_]COND_EXPR stmt, thus
>
> _1 = @0 cmp @1;
> _2 = _1 ? @2 : @3;
>
> that's something we can eventually fix though.  At least it handles
> both cases in pattern recognition (embedded GENERIC or SSA name).
>
> Note the above still misses the case where @2 and @3 are swapped
> so you'd have to duplicate things once more.  You also have to
> be careful to omit comparison codes that will not yield in a
> combination that can be expressed in a single comparison.
>
> I also noticed the comment I added:
>
> /* ???  This matches embedded conditions open-coded because genmatch
>    would generate matching code for conditions in separate stmts only.
>    The following is still important to merge then and else arm cases
>    from if-conversion.  */
>
> which is techincally wrong - the matching code is just unfortunate
> (eh...  I'll see to fix that):
>
>            switch (gimple_assign_rhs_code (def))
>              {
>              case LT_EXPR:
>                {
> ... SSA name condition handling ...
>                  break;
>                }
>              case LT_EXPR:    <--- oops ;)
>                {
> ... GENERIC conditon handling ...
>                    }
>                  break;
>
> Stupid GIMPLE IL ...
>
> As a general remark I think handling of this simplification is
> better done in the reassoc pass (see Jakubs comment #4) given
> || and && associate.  So I'd rather go down that route if possible.

Still, if there are some easy cases that can be handled early in match.pd, 
that can't hurt... (if there aren't, that's fine)
Just like x+y+x -> 2*x+y is for reassoc, but x+x -> 2*x can be done in 
match.pd.

--
Marc Glisse