Re: [PATCH] gimple-match: Do not try UNCOND optimization with COND_LEN.

[Robin Dapp <rdapp.gcc@gmail.com>]

> Why are the contents of this if statement wrong for COND_LEN?
> If the "else" value doesn't matter, then the masked form can use
> the "then" value for all elements.  I would have expected the same
> thing to be true of COND_LEN.

Right, that one was overly pessimistic.  Removed.

> But isn't the test whether res_op->code itself is an internal_function?
> In other words, shouldn't it just be:
> 
>       if (internal_fn_p (res_op->code)
> 	  && internal_fn_len_index (as_internal_fn (res_op->code)) != -1)
> 	return true;
> 
> maybe_resimplify_conditional_op should already have converted to an
> internal function where possible, and if combined_fn (res_op->code)
> does any extra conversion on the fly, that conversion won't be reflected
> in res_op.

I went through some of our test cases and believe most of the problems
are due to situations like the following:

In vect-cond-arith-2.c we have (on riscv)
  vect_neg_xi_14.4_23 = -vect_xi_13.3_22;
  vect_res_2.5_24 = .COND_LEN_ADD ({ -1, ... }, vect_res_1.0_17, vect_neg_xi_14.4_23, vect_res_1.0_17, _29, 0);

On aarch64 this is a situation that matches the VEC_COND_EXPR
simplification that I disabled with this patch.  We valueized
to _26 = vect_res_1.0_17 - vect_xi_13.3_22 and then create
vect_res_2.5_24 = VEC_COND_EXPR <loop_mask_22, _26, vect_res_1.0_19>;
This is later re-assembled into a COND_SUB.

As we have two masks or COND_LEN we cannot use a VEC_COND_EXPR to
achieve the same thing.  Would it be possible to create a COND_OP
directly instead, though?  I tried the following (not very polished
obviously):

-      new_op.set_op (VEC_COND_EXPR, res_op->type,
-                    res_op->cond.cond, res_op->ops[0],
-                    res_op->cond.else_value);
-      *res_op = new_op;
-      return gimple_resimplify3 (seq, res_op, valueize);
+      if (!res_op->cond.len)
+       {
+         new_op.set_op (VEC_COND_EXPR, res_op->type,
+                        res_op->cond.cond, res_op->ops[0],
+                        res_op->cond.else_value);
+         *res_op = new_op;
+         return gimple_resimplify3 (seq, res_op, valueize);
+       }
+      else if (seq && *seq && is_gimple_assign (*seq))
+       {
+         new_op.code = gimple_assign_rhs_code (*seq);
+         new_op.type = res_op->type;
+         new_op.num_ops = gimple_num_ops (*seq) - 1;
+         new_op.ops[0] = gimple_assign_rhs1 (*seq);
+         if (new_op.num_ops > 1)
+           new_op.ops[1] = gimple_assign_rhs2 (*seq);
+         if (new_op.num_ops > 2)
+           new_op.ops[2] = gimple_assign_rhs2 (*seq);
+
+         new_op.cond = res_op->cond;
+
+         gimple_match_op bla2;
+         if (convert_conditional_op (&new_op, &bla2))
+           {
+             *res_op = bla2;
+             // SEQ should now be dead.
+             return true;
+           }
+       }

This would make the other hunk (check whether it was a LEN
and try to recreate it) redundant I hope.

I don't know enough about valueization, whether it's always
safe to do that and other implications.  On riscv this seems
to work, though and the other backends never go through the LEN
path.  If, however, this is a feasible direction it could also
be done for the non-LEN targets?

Regards
 Robin