Re: [PATCH VECT]Support operand swapping for cond_expr in vect_slp

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Oct 27, 2016 at 3:37 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
> Hi,
> During analysis, vect_slp checks if statements of a group are isomorphic to each other, specifically, all statements have to be isomorphic to the first one.  Apparently, operands of commutative operators (PLUS_EXPR/MINUS_EXPR etc.) could be swapped when checking isomorphic property.  Though vect_slp has basic support for such commutative operators, the related code is not properly implemented:
>   1) vect_build_slp_tree mixes operand swapping in the current slp tree node and operand swapping in its child slp tree nodes.
>   2) Operand swapping in the current slp tree is incorrect when vect_get_and_check_slp_defs has already committed to a fixed operand order.
> In addition, operand swapping for COND_EXPR is implemented in a wrong way (place) because:
>   3) vect_get_and_check_slp_defs swaps operands for COND_EXPR by changing comparison code after vect_build_slp_tree_1 checks the code consistency for the statement group.
>   4) vect_build_slp_tree_1 should support operand swapping for COND_EXPR while it doesn't.
>
> This patch addresses above issues.  It supports COND_EXPR by recording swapping information in vect_build_slp_tree_1 and applies the swap in vect_get_check_slp_defs.  It supports two types swapping: swapping and inverting.  The patch also does refactoring so that operand swapping in child slp tree node and the current slp tree node are differentiated.  With this patch, failures (gcc.dg/vect/slp-cond-3.c) revealed by previous COND_EXPR match.pd patches are resolved.
> Bootstrap and test on x86_64 and AArch64.  Is it OK?

Ok, but please re-instantiate the early-out here:

@@ -905,18 +960,10 @@ vect_build_slp_tree (vec_info *vinfo,
   slp_oprnd_info oprnd_info;
   FOR_EACH_VEC_ELT (stmts, i, stmt)
     {
-      switch (vect_get_and_check_slp_defs (vinfo, stmt, i, &oprnds_info))
-       {
-       case 0:
-         break;
-       case -1:
-         matches[0] = false;
-         vect_free_oprnd_info (oprnds_info);
-         return NULL;

^^^^

you seem to needlessly continue checking other DEFs if one returns -1.

Thanks,
Richard.

> Thanks,
> bin
>
> 2016-10-25  Bin Cheng  <bin.cheng@arm.com>
>
>         * tree-vect-slp.c (vect_get_and_check_slp_defs): New parameter SWAP.
>         Check slp defs for COND_EXPR by swapping/inverting operands if
>         indicated by the new parameter SWAP.
>         (vect_build_slp_tree_1): New parameter SWAP.  Check COND_EXPR stmt
>         is isomorphic to the first stmt via swapping/inverting.  Store swap
>         information in the new parameter SWAP.
>         (vect_build_slp_tree): New local array SWAP and pass it to function
>         vect_build_slp_tree_1.  Cleanup result handlding code for function
>         call to vect_get_and_check_slp_defs.  Skip oeprand swapping if the
>         order of operands has been fixed as indicated by SWAP[i].