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

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Wed, Jun 03, 2020 at 04:46:12PM +0200, Richard Biener wrote:
> On Wed, Jun 3, 2020 at 4:17 PM David Edelsohn <dje.gcc@gmail.com> wrote:
> > On Wed, Jun 3, 2020 at 9:41 AM Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> > > Well, it seems unfortunate to have to do that.
> > >
> > > I think Martin's powerpc patch is the correct one.

It is papering over the issues a little -- the same assumption is made
at lower levels as well, so all *that* needs to be changed as well (not
"fixed", it is not a bug, we have a change in the vcond* interface here;
oh and that should be documented as well).

> > How about (3) help to remove reliance on this incorrect behavior from
> > the PowerPC port?

It is not a reliance on incorrect behaviour.  This is a change.  Which
pretty much everyone seems to want, so fine, but that takes time.

> > I didn't formally check, but if this is 16 years old, then it's from
> > the original RHES Altivec work.

It is, exactly.

> > I don't believe that anyone fundamentally is objecting to "fixing this
> > correctly".  I don't know the entire history of this discussion, but
> > my objection is to a fix that breaks a long-time assumption of the
> > PowerPC port and leaves it as an exercise to the PowerPC maintainers
> > to fix it.

*Exactly*.  This is changing an ancient interface, claiming "it always
was that way" (which very obviously isn't true), and leaving the rs6000
people to deal with all the fallout.  Again.

> I _think_ there's nothing to fix besides removing the FAIL.

All the lower levels need to get asserts as well.  We need a week or so
to put the whole thing through the wringer.  The documentation needs to
be changed by whoever changes the vcond* semantics.  All other ports
should be checked, too.

> And I would
> have no idea how to "fix" the powerpc port here since a) we lack a testcase
> that actually FAILs, b) I'm not familiar with the ISA.  So we did (3) by
> replacing the FAILs with gcc_unreachable () and bootstrap/regtest this
> without any regression which I think "proves" the failure modes do not
> actually exist.

Heh, assuming the testsuite is comprehensive?  Heh.  (Bootstrap doesn't
mean much for vector code).

> So I'm not sure how we can help.

You'll have to document the "vcond* is not allowed to FAIL" change.
We'll deal with the rest.  But testing needs a week or so.  (That is
an extremely short timescale already).

> A vcond can usually be emulated by vec_cmp plus masking.

That would be the generic way to implement this of course, but apparently
such code doesn't yet exist?  If there is a generic implementation it
should be trivial to deal with FAILs.

> So if
> we ever get a testcase that runs into the gcc_unreachable () I'll promise
> to fix it up using this strategy in the vcond expander.  But without a
> testcase and powerpc ISA knowledge it's really hard.  Or do you want
> us to stick the vec_cmp expansion fallback in place of the FAILs?
> I'm sure the powerpc maintainers are better suited to do that even though
> I'll probably manage with some cut&paste.  To recap: vcond is
> equal to
> 
>   mask = vec_cmp of the comparison
>   true_masked = true_op & mask;
>   false_masked = false_op & ~mask;
>   result = true_masked | false_masked;
> 
> but I believe this would be dead code never triggered.

But that would be the generic code as well?  Is that not useful to have
in any case?


Segher