Re: [PATCH 00/57] Replace the Power target-specific built-in machinery

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

Hi!

Just a few small things about this -- I'll reply to more of it later.

On Tue, Apr 27, 2021 at 10:32:35AM -0500, Bill Schmidt via Gcc-patches wrote:
> The design of the target-specific built-in function support in the
> Power back end has not stood the test of time.  The machinery is
> grossly inefficient, confusing, and arcane; and adding new built-in
> functions is inefficient and error-prone.

You are too nice to it.  People have had to work with it over the
years, there is some pent-up anger :-)

> Because of the scope of the changes, it's important to be able to
> verify that the new system makes only intended changes to the
> functions that are supported.  Therefore this patch set adds a new
> mechanism, and (in the final patch) enables it instead of the existing
> support, but does not yet remove the old support.  That will happen in
> a follow-up patch once we're comfortable with the new system.

Is there some (semi-)automatic way to compare the results of the old
and new systems?

> Patch 0057: Fix one last late-breaking change
> 
>   Keeping the code up-to-date with upstream has been fun.  When I
>   rebased to create the patch set, I found one new issue where a
>   small change had been made to the overload handling for the
>   vec_insert builtins.  This patch reflects that change into the
>   new handling.  My version of git is having trouble with
>   interactive rebasing, so it was easier to just add the extra patch.

What breaks by keeping this fix after the other patches?

> I deliberately implemented all the old built-ins exactly as previously
> defined, wherever possible, despite an overwhelming desire to pitch
> out a bunch of them that have already been considered deprecated for
> ages.  I found that it was too difficult to both implement a new
> system and remove deprecated things at the same time, and in general
> it seems like a dangerous thing to do.  Better to do this in stages if
> we're going to do it at all.

Independent fixes you can put before the meat of the series.  This often
is the best way to do it, since then you don't have to duplicate the
weird / buggy / whatever behaviour of the old system.

But things that aren't simple fixes, that need deprecation periods and
everything...  no no no, you want this done *this* decade!

> Unfortunately a lot of deprecated things
> still appear all over our own test suite, and I'm afraid we can assume
> they appear in user code all over the place as well.

Pretty much the only old features you can remove are features that have
been broken for many years.  You can break something on purpose to see
if anyone still uses it, too, but :-)

> What I've done instead is to make very clear which interfaces are
> considered deprecated in the input files themselves.  Over time,
> perhaps we can start to remove some of these, but in reality I think
> we're going to just continue to be stuck with them.

It is extremely useful to have it clearly documented which interfaces
shoul;d be considered deprecated, even if we will never remove it.  It
is useful in the documentation, but it is even more useful in the code,
for ourselves!

> (3) A number of built-ins used "long" for DImode types, which would
> break these for 32-bit.  I changed those arguments and return values
> to "long long" to avoid such problems, when those built-ins were not
> restricted to 64-bit mode already.  There aren't many such cases.

You can do this for 64-bit-only builtins as well -- the actual argument
type is never visible (to the user), and everything becomes modes early.


Segher