Re: [stage1][PATCH] Lower VEC_COND_EXPR into internal functions.

[Richard Biener <richard.guenther@gmail.com>]

On Fri, May 29, 2020 at 2:17 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 5:28 PM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > Martin Liška <mliska@suse.cz> writes:
> > > Hi.
> > >
> > > There's a new patch that adds normal internal functions for the 4
> > > VCOND* functions.
> > >
> > > The patch that survives bootstrap and regression
> > > tests on x86_64-linux-gnu and ppc64le-linux-gnu.
> >
> > I think this has the same problem as the previous one.  What I meant
> > in yesterday's message is that:
> >
> >   expand_insn (icode, 6, ops);
> >
> > is simply not valid when icode is allowed to FAIL.  That's true in
> > any context, not just internal functions.  If icode does FAIL,
> > the expand_insn call will ICE:
> >
> >   if (!maybe_expand_insn (icode, nops, ops))
> >     gcc_unreachable ();
> >
> > When using optabs you either:
> >
> > (a) declare that the md patterns aren't allowed to FAIL.  expand_insn
> >     is for this case.
> >
> > (b) allow the md patterns to FAIL and provide a fallback when they do.
> >     maybe_expand_insn is for this case.
> >
> > So if we keep IFN_VCOND, we need to use maybe_expand_insn and find some
> > way of implementing the IFN_VCOND when the pattern FAILs.
>
> But we should not have generated the pattern in that case - we actually verify
> we can expand at the time we do this "instruction selection".  This is in-line
> with other vectorizations where we also do not expect things to FAIL.
>
> See also the expanders that are removed in the patch.
>
> But adding a comment in the internal function expander to reflect this
> is probably good, also pointing to the verification routines (the
> preexisting expand_vec_cond_expr_p and expand_vec_cmp_expr_p
> routines).  Because of this pre-verification I suggested the direct
> internal function first, not being aware of the static cannot-FAIL logic.
>
> Now it looks like that those verification also simply checks optab
> availability only but then this is just a preexisting issue (and we can
> possibly build a testcase that FAILs RTL expansion for power...).
>
> So given that this means the latent bug in the powerpc backend
> should be fixed and we should use a direct internal function instead?

So I tried to understand the circumstances the rs6000 patterns FAIL
but FAILed ;)  It looks like some outs of rs6000_emit_vector_cond_expr
are unwarranted and the following should work:

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 8435bc15d72..5503215a00a 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -14638,8 +14638,7 @@ rs6000_emit_vector_compare (enum rtx_code rcode,
        rtx mask2;

        rev_code = reverse_condition_maybe_unordered (rcode);
-       if (rev_code == UNKNOWN)
-         return NULL_RTX;
+       gcc_assert (rev_code != UNKNOWN);

        nor_code = optab_handler (one_cmpl_optab, dmode);
        if (nor_code == CODE_FOR_nothing)
@@ -14737,8 +14736,7 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx
op_true, rtx op_false,
   rtx cond2;
   bool invert_move = false;

-  if (VECTOR_UNIT_NONE_P (dest_mode))
-    return 0;
+  gcc_assert (VECTOR_UNIT_NONE_P (dest_mode));

   gcc_assert (GET_MODE_SIZE (dest_mode) == GET_MODE_SIZE (mask_mode)
              && GET_MODE_NUNITS (dest_mode) == GET_MODE_NUNITS (mask_mode));
@@ -14756,8 +14754,7 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx
op_true, rtx op_false,
         e.g., A  = (B != C) ? D : E becomes A = (B == C) ? E : D.  */
       invert_move = true;
       rcode = reverse_condition_maybe_unordered (rcode);
-      if (rcode == UNKNOWN)
-       return 0;
+      gcc_assert (rcode != UNKNOWN);
       break;

     case GE:

which leaves the

  /* Get the vector mask for the given relational operations.  */
  mask = rs6000_emit_vector_compare (rcode, cc_op0, cc_op1, mask_mode);

  if (!mask)
    return 0;

fail but that function recurses heavily - from reading
rs6000_emit_vector_compare_inner
it looks like power can do a lot of compares but floating-point LT which
reverse_condition_maybe_unordered would turn into UNGE which is not
handled either.
But then rs6000_emit_vector_compare just tries GT for that anyway (not UNGE) so
it is actually be handled (but should not?).

So I bet the expansion of the patterns cannot fail at the moment.  Thus I'd
replace the FAIL with a gcc_unreachable () and see if we have test
coverage for those
FAILs.

Segher - do you actually know this code to guess why the patterns are defensive?

Thanks,
Richard.

> Thanks,
> Richard.
>
> > Thanks,
> > Richard