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

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

On Sat, May 30, 2020 at 3:08 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Hi!
>
> On Sat, May 30, 2020 at 08:15:55AM +0100, Richard Sandiford wrote:
> > Segher Boessenkool <segher@kernel.crashing.org> writes:
> > >> Sure.  But the point is that FAILing isn't “explicitly allowed” for vcond*.
> > >> In fact it's the opposite.
>
> I disagree btw, and no one else has noticed for 16 years either.
>
> In general, almost all patterns can FAIL, and those that can not are
> simply because no one wrote fallback code.  Which means that all
> targets that need a fallback need to implement the same thing for
> themselves, which is just a waste and causes extra errors.
>
> So, "cannot FAIL" should be a temporary thing, and should change to
> "can FAIL" as soon as someone implements that, and never be changed
> back -- and it should be how almost everything is in the first place
> (and it still is, thankfully).
>
> > > It has FAILed on rs6000 since 2004.
> >
> > But that just means that the powerpc bug has been there since 2004,
> > assuming these FAILs can actually trigger in practice.  At that time,
> > the corresponding expand code was:
>
> I, and I think most other people, thought it was allowed to FAIL (and
> I still do).
>
> > rtx
> > expand_vec_cond_expr (tree vec_cond_expr, rtx target)
>
> [ snip ]
>
> So this was buggy.
>
> > i.e. no fallbacks, and no checking whether the expansion even
> > succeeded.  Since FAIL just causes the generator to return null,
> > and since emit_insn is a no-op for null insns, the effect for
> > FAILs was to emit no instructions and return an uninitialised
> > target register.
> >
> > The silent use of an uninitialised register was changed in 2011
> > to an ICE, via the introduction of expand_insn.
>
> Yeah, I ran into some of that in 2015, at least then not all of that
> was fixed.  That was some very basic insn I think, that really should
> never fail, a simple branch or something...  Was surprising though, a
> good reminder to always check return values :-)
>
> > The fact that we've had no code to handle the FAILs for 15+ years
> > without apparent problems makes it even more likely that the FAILs
> > never happen in practice.
>
> AltiVec can do a lot less than VSX (and VSX on p7 can do less than on
> p8, and that can do less than p9, etc.), so I am pretty certain it
> could fail for some cases.  Only up to not so very long ago these
> patterns were mainly (or only?) used via builtins, and the code for
> those handles all those cases already.
>
> > If you think the FAILs do trigger in practice, please provide an example.
>
> As I said before, that is completely beside the point.
>
> vcond is allowed to FAIL.  No pattern that can FAIL should ever be
> changed to not allow that anymore.  This would make no sense at all.

Fact is if the FAIL happens we ICE _currently_.

The patterns may not FAIL since otherwise the vectorizer has no way
to check whether the backend can code-generate and fail vectorization
if not.  FAIL is a _very_ unspecific fail and I guess the middle-end would
need to cope with a pattern (considered as may-FAIL) doing

  if (random() == 5)
    FAIL;

specifically not FAIL when invoked during vectorization but FAIL when
re-invoked during RTL expansion just because some internal state
at the point of RTL expansion cannot be simulated at vectorization time.

Now the real issue here is of course that if vcond expansion may
FAIL then of course, based on your reasoning, vec_cmp expansion
may so as well.  In that case there's _no_ way to code generate
either of the two.  Well, spill, do elementwise compare (are cmp*
allowed to FAIL?), store, load would do the trick - but that defeats
the attempt to cost during vectorization.

So please be constructive.  Like, provide a testcase that ICEs
with the FAILs replaced by gcc_unreachable ().  Martin, may I suggest
to do this replacement and bootstrap/test?  I think it would be nice
to have testsuite coverage for the FAILs, and maybe we have that
already.

To get out of the deadlock here I'll approve a patch variant that
uses a separate internal function (thus without the static non-FAIL
checking) that preserves current behavior - thus ICE if the pattern
FAILs.  This change is then not a regression.  (please re-post for
appropriate approval)

Unless we come to a consensus in this discussion which seems
to dance around the latent vectorizer <-> rs6000 interface issue.

CCing David as other rs6000 maintainer (the subject isn't
specifically pointing to rs6000 so not sure if you followed the
discussion).

Thanks,
Richard.

>
> Segher