On Fri, May 29, 2020 at 2:17 PM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 5:28 PM Richard Sandiford
> <richard.sandif...@arm.com> wrote:
> >
> > Martin Liška <mli...@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
