Re: [PATCH] combine: Try harder to form zero_extends [PR106594]

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

Hi!

On Mon, Mar 06, 2023 at 07:13:08PM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > Most importantly, what makes you think this is a problem for aarch64
> > only?  If it actually is, you can fix it in the aarch64 config!  Either
> > with or without new hooks, whatever works best.
> 
> The point is that I don't think it's a problem for AArch64 only.
> I think it's a generic issue that should be solved in a generic way
> (which is what the patch is trying to do).  The suggestion to restrict
> it to AArch64 came from Jakub.
> 
> The reason I'm pushing back against a hook is precisely because
> I don't want to solve this in AArch64-specific code.

But it is many times worse still to do it in target-specific magic code
disguised as generic code :-(

If there is no clear explanation why combine should do X, then it
probably should not.

> I'm not sure we would be talking about restricting this to AArch64
> if the patch had been posted in stage 1.  If people are concerned
> about doing this for all targets in stage 4 (which they seem to be),

Not me, not in principle.  But it takes more time than we have left in
stage 4 to handle this, even for only combine.  We should give the other
target maintainers much longer as well.

> I thought the #ifdef was the simplest way of addressing that concern.

An #ifdef is a way of making a change that is not finished yet not hurt
the other targets.  It still hurts generic development, which indirectly
hurts all targets.

> And I don't think what the patch does is ad hoc.

It is almost impossible to explain what it does and why that is a good
thing, why it is what we want, what we should do here; and certainly not
in a compact, terse, focused way.  It has all the hallmarks of ad hoc
patches.

> Reorganising the
> expression in this way isn't something new.  extract_left_shift already
> does a similar thing (and does it for all targets).

That is not similar at all, no.

/* See if X (of mode MODE) contains an ASHIFT of COUNT or more bits that
   can be commuted with any other operations in X.  Return X without
   that shift if so.  */

If you can factor out a utility function like that, with an actual nice
description like that, it would be a much more palatable patch.


Segher