Re: [PATCH, match] Fix pr68714

[Richard Biener <rguenther at suse dot de>]

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

+           (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.

Thanks,
Richard.